Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(improve): Remove scan through globals #6282

Merged
merged 9 commits into from
Oct 11, 2024
Merged

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Oct 11, 2024

Description

Problem*

Resolves #5001

Summary*

Removes the search for a global by local module ID and name when elaborating a global variable declaration. Instead simply looks up the global by the ID that we have in the definition we are elaborating. In fact the elaboration starts from a global ID of an unresolved global.

I could be way off the mark on whether this is the right thing to do.

Additional Context

I found a couple of things odd:

  • NodeInterner::push_global pushes a DefinitionKind::Global(id) with the exact ID that the GlobalInfo will have, ie. it is self-referential (just an observation). This is called during the collection of unresolved globals.
  • Elaborator::add_global_variable_decl only looks for globals by local_id and name, but not by definition, even though at this point we know that the definition must contain a GlobalId, otherwise we would have stayed in Elaborator::add_variable_decl.
  • if a global cannot be found, it calls NodeInterner::push_definition with the definition in the parameters; since we know it contains a GlobalId and that the same ID must have been inserted in push_global as well, it will now be part of two definitions at different locations - not sure exactly what this means or looks like in code

I added some assertions in the commits to check that during the tests on CI, the global_id and the definition, and any results found by the lookup by local+name, are always consistent, which is why in the end I thought that if the only thing we can find is the one we can get by ID, why not get just get it?

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@aakoshh aakoshh changed the title Maintain a reverse index from (LocalModuleId, Ident) to GlobalId feat(improve): Index globals by module and ident for faster lookups Oct 11, 2024
@aakoshh aakoshh marked this pull request as ready for review October 11, 2024 13:47
@aakoshh aakoshh changed the title feat(improve): Index globals by module and ident for faster lookups improve: Index globals by module and ident for faster lookups Oct 11, 2024
@aakoshh aakoshh changed the title improve: Index globals by module and ident for faster lookups feat(improve): Index globals by module and ident for faster lookups Oct 11, 2024
@aakoshh aakoshh changed the title feat(improve): Index globals by module and ident for faster lookups feat(improve): Remove scan through globals Oct 11, 2024
aakoshh added a commit that referenced this pull request Oct 11, 2024
…it from the definition (#6283)

# Description

## Problem\*

Followup to #6282

## Summary\*
Some methods in the `Elaborator` received a `global_id:
Option<GlobalId>` and a `definition: DefinitionKind` parameter. We found
that `Elaborator::elaborate_let` actually created `definition` to carry
the same global ID as it was given, so `global_id: Some(id)` and
`definition: DefinitionKind::Global(id)` always carried the same value.
We added an
[assertion](1d2bd36)
that checked that they are never different.

The next logical step is to remove the possibility of confusion by only
carrying the ID in the definition.


## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
…it from the definition (#6283)

# Description

## Problem\*

Followup to #6282

## Summary\*
Some methods in the `Elaborator` received a `global_id:
Option<GlobalId>` and a `definition: DefinitionKind` parameter. We found
that `Elaborator::elaborate_let` actually created `definition` to carry
the same global ID as it was given, so `global_id: Some(id)` and
`definition: DefinitionKind::Global(id)` always carried the same value.
We added an
[assertion](1d2bd36)
that checked that they are never different.

The next logical step is to remove the possibility of confusion by only
carrying the ID in the definition.

## Additional Context

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
Copy link
Contributor

@michaeljklein michaeljklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jfecher jfecher added this pull request to the merge queue Oct 11, 2024
Merged via the queue into master with commit fd91913 Oct 11, 2024
47 checks passed
@jfecher jfecher deleted the 5001-global-index branch October 11, 2024 16:44
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 12, 2024
feat(improve): Remove scan through globals (noir-lang/noir#6282)
feat: show LSP diagnostic related information (noir-lang/noir#6277)
feat: trait inheritance (noir-lang/noir#6252)
feat: optimize reading a workspace's files (noir-lang/noir#6281)
fix: prevent compiler panic when popping from empty slices (noir-lang/noir#6274)
feat(test): Include the PoseidonHasher in the fuzzing (noir-lang/noir#6280)
feat: slightly improve "unexpected token" error message (noir-lang/noir#6279)
feat(test): Fuzz poseidon hases against an external library (noir-lang/noir#6273)
feat: remove byte decomposition in `compute_decomposition` (noir-lang/noir#6159)
fix: address inactive public key check in `verify_signature_noir` (noir-lang/noir#6270)
feat(test): Fuzz test poseidon2 hash equivalence (noir-lang/noir#6265)
fix!: Integer division is not the inverse of integer multiplication (noir-lang/noir#6243)
feat(perf): Flamegraphs for test program execution benchmarks (noir-lang/noir#6253)
fix: visibility for impl methods (noir-lang/noir#6261)
feat: Add `checked_transmute` (noir-lang/noir#6262)
feat!: kind size checks (noir-lang/noir#6137)
feat: don't crash LSP when there are errors resolving the workspace (noir-lang/noir#6257)
fix: don't warn on unuse global if it has an abi annotation (noir-lang/noir#6258)
fix: don't warn on unused struct that has an abi annotation (noir-lang/noir#6254)
feat: don't suggest private struct fields in LSP (noir-lang/noir#6256)
feat: visibility for struct fields (noir-lang/noir#6221)
fix: handle dfg databus in SSA normalization (noir-lang/noir#6249)
fix: homogeneous input points for EC ADD (noir-lang/noir#6241)
chore: add regression test for #5756 (noir-lang/noir#5770)
feat: allow `unconstrained` after visibility (noir-lang/noir#6246)
feat: optimize `Quoted::as_expr` by parsing just once (noir-lang/noir#6237)
fix(frontend): Do not warn when a nested struct is provided as input to main (noir-lang/noir#6239)
fix!: Change tag attributes to require a ' prefix (noir-lang/noir#6235)
feat: recover from '=' instead of ':' in struct constructor/pattern (noir-lang/noir#6236)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 12, 2024
feat(improve): Remove scan through globals (noir-lang/noir#6282)
feat: show LSP diagnostic related information (noir-lang/noir#6277)
feat: trait inheritance (noir-lang/noir#6252)
feat: optimize reading a workspace's files (noir-lang/noir#6281)
fix: prevent compiler panic when popping from empty slices (noir-lang/noir#6274)
feat(test): Include the PoseidonHasher in the fuzzing (noir-lang/noir#6280)
feat: slightly improve "unexpected token" error message (noir-lang/noir#6279)
feat(test): Fuzz poseidon hases against an external library (noir-lang/noir#6273)
feat: remove byte decomposition in `compute_decomposition` (noir-lang/noir#6159)
fix: address inactive public key check in `verify_signature_noir` (noir-lang/noir#6270)
feat(test): Fuzz test poseidon2 hash equivalence (noir-lang/noir#6265)
fix!: Integer division is not the inverse of integer multiplication (noir-lang/noir#6243)
feat(perf): Flamegraphs for test program execution benchmarks (noir-lang/noir#6253)
fix: visibility for impl methods (noir-lang/noir#6261)
feat: Add `checked_transmute` (noir-lang/noir#6262)
feat!: kind size checks (noir-lang/noir#6137)
feat: don't crash LSP when there are errors resolving the workspace (noir-lang/noir#6257)
fix: don't warn on unuse global if it has an abi annotation (noir-lang/noir#6258)
fix: don't warn on unused struct that has an abi annotation (noir-lang/noir#6254)
feat: don't suggest private struct fields in LSP (noir-lang/noir#6256)
feat: visibility for struct fields (noir-lang/noir#6221)
fix: handle dfg databus in SSA normalization (noir-lang/noir#6249)
fix: homogeneous input points for EC ADD (noir-lang/noir#6241)
chore: add regression test for #5756 (noir-lang/noir#5770)
feat: allow `unconstrained` after visibility (noir-lang/noir#6246)
feat: optimize `Quoted::as_expr` by parsing just once (noir-lang/noir#6237)
fix(frontend): Do not warn when a nested struct is provided as input to main (noir-lang/noir#6239)
fix!: Change tag attributes to require a ' prefix (noir-lang/noir#6235)
feat: recover from '=' instead of ':' in struct constructor/pattern (noir-lang/noir#6236)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 13, 2024
feat(improve): Remove scan through globals (noir-lang/noir#6282)
feat: show LSP diagnostic related information (noir-lang/noir#6277)
feat: trait inheritance (noir-lang/noir#6252)
feat: optimize reading a workspace's files (noir-lang/noir#6281)
fix: prevent compiler panic when popping from empty slices (noir-lang/noir#6274)
feat(test): Include the PoseidonHasher in the fuzzing (noir-lang/noir#6280)
feat: slightly improve "unexpected token" error message (noir-lang/noir#6279)
feat(test): Fuzz poseidon hases against an external library (noir-lang/noir#6273)
feat: remove byte decomposition in `compute_decomposition` (noir-lang/noir#6159)
fix: address inactive public key check in `verify_signature_noir` (noir-lang/noir#6270)
feat(test): Fuzz test poseidon2 hash equivalence (noir-lang/noir#6265)
fix!: Integer division is not the inverse of integer multiplication (noir-lang/noir#6243)
feat(perf): Flamegraphs for test program execution benchmarks (noir-lang/noir#6253)
fix: visibility for impl methods (noir-lang/noir#6261)
feat: Add `checked_transmute` (noir-lang/noir#6262)
feat!: kind size checks (noir-lang/noir#6137)
feat: don't crash LSP when there are errors resolving the workspace (noir-lang/noir#6257)
fix: don't warn on unuse global if it has an abi annotation (noir-lang/noir#6258)
fix: don't warn on unused struct that has an abi annotation (noir-lang/noir#6254)
feat: don't suggest private struct fields in LSP (noir-lang/noir#6256)
feat: visibility for struct fields (noir-lang/noir#6221)
fix: handle dfg databus in SSA normalization (noir-lang/noir#6249)
fix: homogeneous input points for EC ADD (noir-lang/noir#6241)
chore: add regression test for #5756 (noir-lang/noir#5770)
feat: allow `unconstrained` after visibility (noir-lang/noir#6246)
feat: optimize `Quoted::as_expr` by parsing just once (noir-lang/noir#6237)
fix(frontend): Do not warn when a nested struct is provided as input to main (noir-lang/noir#6239)
fix!: Change tag attributes to require a ' prefix (noir-lang/noir#6235)
feat: recover from '=' instead of ':' in struct constructor/pattern (noir-lang/noir#6236)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 13, 2024
feat(improve): Remove scan through globals (noir-lang/noir#6282)
feat: show LSP diagnostic related information (noir-lang/noir#6277)
feat: trait inheritance (noir-lang/noir#6252)
feat: optimize reading a workspace's files (noir-lang/noir#6281)
fix: prevent compiler panic when popping from empty slices (noir-lang/noir#6274)
feat(test): Include the PoseidonHasher in the fuzzing (noir-lang/noir#6280)
feat: slightly improve "unexpected token" error message (noir-lang/noir#6279)
feat(test): Fuzz poseidon hases against an external library (noir-lang/noir#6273)
feat: remove byte decomposition in `compute_decomposition` (noir-lang/noir#6159)
fix: address inactive public key check in `verify_signature_noir` (noir-lang/noir#6270)
feat(test): Fuzz test poseidon2 hash equivalence (noir-lang/noir#6265)
fix!: Integer division is not the inverse of integer multiplication (noir-lang/noir#6243)
feat(perf): Flamegraphs for test program execution benchmarks (noir-lang/noir#6253)
fix: visibility for impl methods (noir-lang/noir#6261)
feat: Add `checked_transmute` (noir-lang/noir#6262)
feat!: kind size checks (noir-lang/noir#6137)
feat: don't crash LSP when there are errors resolving the workspace (noir-lang/noir#6257)
fix: don't warn on unuse global if it has an abi annotation (noir-lang/noir#6258)
fix: don't warn on unused struct that has an abi annotation (noir-lang/noir#6254)
feat: don't suggest private struct fields in LSP (noir-lang/noir#6256)
feat: visibility for struct fields (noir-lang/noir#6221)
fix: handle dfg databus in SSA normalization (noir-lang/noir#6249)
fix: homogeneous input points for EC ADD (noir-lang/noir#6241)
chore: add regression test for #5756 (noir-lang/noir#5770)
feat: allow `unconstrained` after visibility (noir-lang/noir#6246)
feat: optimize `Quoted::as_expr` by parsing just once (noir-lang/noir#6237)
fix(frontend): Do not warn when a nested struct is provided as input to main (noir-lang/noir#6239)
fix!: Change tag attributes to require a ' prefix (noir-lang/noir#6235)
feat: recover from '=' instead of ':' in struct constructor/pattern (noir-lang/noir#6236)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 14, 2024
feat(improve): Remove scan through globals (noir-lang/noir#6282)
feat: show LSP diagnostic related information (noir-lang/noir#6277)
feat: trait inheritance (noir-lang/noir#6252)
feat: optimize reading a workspace's files (noir-lang/noir#6281)
fix: prevent compiler panic when popping from empty slices (noir-lang/noir#6274)
feat(test): Include the PoseidonHasher in the fuzzing (noir-lang/noir#6280)
feat: slightly improve "unexpected token" error message (noir-lang/noir#6279)
feat(test): Fuzz poseidon hases against an external library (noir-lang/noir#6273)
feat: remove byte decomposition in `compute_decomposition` (noir-lang/noir#6159)
fix: address inactive public key check in `verify_signature_noir` (noir-lang/noir#6270)
feat(test): Fuzz test poseidon2 hash equivalence (noir-lang/noir#6265)
fix!: Integer division is not the inverse of integer multiplication (noir-lang/noir#6243)
feat(perf): Flamegraphs for test program execution benchmarks (noir-lang/noir#6253)
fix: visibility for impl methods (noir-lang/noir#6261)
feat: Add `checked_transmute` (noir-lang/noir#6262)
feat!: kind size checks (noir-lang/noir#6137)
feat: don't crash LSP when there are errors resolving the workspace (noir-lang/noir#6257)
fix: don't warn on unuse global if it has an abi annotation (noir-lang/noir#6258)
fix: don't warn on unused struct that has an abi annotation (noir-lang/noir#6254)
feat: don't suggest private struct fields in LSP (noir-lang/noir#6256)
feat: visibility for struct fields (noir-lang/noir#6221)
fix: handle dfg databus in SSA normalization (noir-lang/noir#6249)
fix: homogeneous input points for EC ADD (noir-lang/noir#6241)
chore: add regression test for #5756 (noir-lang/noir#5770)
feat: allow `unconstrained` after visibility (noir-lang/noir#6246)
feat: optimize `Quoted::as_expr` by parsing just once (noir-lang/noir#6237)
fix(frontend): Do not warn when a nested struct is provided as input to main (noir-lang/noir#6239)
fix!: Change tag attributes to require a ' prefix (noir-lang/noir#6235)
feat: recover from '=' instead of ':' in struct constructor/pattern (noir-lang/noir#6236)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 14, 2024
feat(improve): Remove scan through globals (noir-lang/noir#6282)
feat: show LSP diagnostic related information (noir-lang/noir#6277)
feat: trait inheritance (noir-lang/noir#6252)
feat: optimize reading a workspace's files (noir-lang/noir#6281)
fix: prevent compiler panic when popping from empty slices (noir-lang/noir#6274)
feat(test): Include the PoseidonHasher in the fuzzing (noir-lang/noir#6280)
feat: slightly improve "unexpected token" error message (noir-lang/noir#6279)
feat(test): Fuzz poseidon hases against an external library (noir-lang/noir#6273)
feat: remove byte decomposition in `compute_decomposition` (noir-lang/noir#6159)
fix: address inactive public key check in `verify_signature_noir` (noir-lang/noir#6270)
feat(test): Fuzz test poseidon2 hash equivalence (noir-lang/noir#6265)
fix!: Integer division is not the inverse of integer multiplication (noir-lang/noir#6243)
feat(perf): Flamegraphs for test program execution benchmarks (noir-lang/noir#6253)
fix: visibility for impl methods (noir-lang/noir#6261)
feat: Add `checked_transmute` (noir-lang/noir#6262)
feat!: kind size checks (noir-lang/noir#6137)
feat: don't crash LSP when there are errors resolving the workspace (noir-lang/noir#6257)
fix: don't warn on unuse global if it has an abi annotation (noir-lang/noir#6258)
fix: don't warn on unused struct that has an abi annotation (noir-lang/noir#6254)
feat: don't suggest private struct fields in LSP (noir-lang/noir#6256)
feat: visibility for struct fields (noir-lang/noir#6221)
fix: handle dfg databus in SSA normalization (noir-lang/noir#6249)
fix: homogeneous input points for EC ADD (noir-lang/noir#6241)
chore: add regression test for #5756 (noir-lang/noir#5770)
feat: allow `unconstrained` after visibility (noir-lang/noir#6246)
feat: optimize `Quoted::as_expr` by parsing just once (noir-lang/noir#6237)
fix(frontend): Do not warn when a nested struct is provided as input to main (noir-lang/noir#6239)
fix!: Change tag attributes to require a ' prefix (noir-lang/noir#6235)
feat: recover from '=' instead of ':' in struct constructor/pattern (noir-lang/noir#6236)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid O(N^2) scan through globals during name resolution
3 participants