Conversation
update from main
update from master
update from main
update from main
update from main
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d2be0a1ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Fallback for while-style loops: use header statements | ||
| let init_docs = self.render_statements(&block.insts, state)?; | ||
| let init_inline = docs_inline(&init_docs); | ||
| if init_inline.is_empty() { | ||
| "{ }".to_string() |
There was a problem hiding this comment.
Recompute while-loop header statements each iteration
For while-style loops (LoopInfo without a post_block), this fallback emits the header block statements only in the init block, so any header computations (e.g., BindValue temps or function calls used by the condition) run once and the condition reuses stale values on later iterations. That changes semantics for cases like while foo() or conditions depending on values recomputed in the header, potentially making loops infinite or skipping iterations; you likely need to re-emit header statements in the post block (as before) or ensure the condition is re-evaluated each iteration.
Useful? React with 👍 / 👎.
| // Collect data regions from reachable functions | ||
| for dr in &info.data_regions { | ||
| if seen_labels.insert(dr.label.clone()) { | ||
| data_regions.push(dr.clone()); |
There was a problem hiding this comment.
Ensure data section labels are unique across functions
Data region labels are generated per MirBody (e.g., data_0, data_1), but here you dedupe data sections across all reachable functions by label, so if two functions both define data_0 with different bytes only the first is emitted and other functions’ datacopy will reference the wrong bytes. Consider making labels globally unique (prefix with symbol or a module-level counter) or remapping labels during collection instead of deduping by label alone.
Useful? React with 👍 / 👎.
|
Continued now on: |
This PR implements for loops down to Yul and also improvements on the current arrays implementation. Design was made after a conversation with @micahscopes about possible needs from Circom's Poseidon Hash if being implemented on Fe.
Happy to discuss the reasons behind the lowering strategies on our call.
While this compiles valid Yul feel free to replace with a better implementation. That being said, I'll keep focusing on the examples and Yul code expected to keep track of the actual implementation.
New Features added
(note that literal ranges are not supported yet)
Notice the #unroll annotation
Will result on:
4.a Immutable arrays. Example here
Now, big immutable arrays will be stored on data region, baked into the bytecode. For example:
Will result on, notice the data region added by encoding the array into a hex string.
4.b Big strings. Example here
Now strings bigger than 32 bytes like this one:
Will be lowered into data region instead of inline stack.
Structural changes
Parser:
..and..=AttrListOwnerparamHIR:
AttrListOwnerfor the #unroll annotationMIR
DataRegionDefadded to the MIR body to accept data regionsCopyDataRegionSynthetic type for arrays and large (>32 bytes) stringsLoopInfoextra data to be able to detect the init and post block for loop codeYul codegen
TODOs