refactor(wasm): emit CASM instead of MIR #343
Conversation
|
Claude finished @gael-bigot's task —— View job Code Review for WASM → CASM RefactorTLDR: This is a substantial architectural improvement that moves WASM compilation directly to CASM, bypassing MIR. The implementation looks solid with good test coverage updates. Main concerns are around error handling robustness and some API design choices. Review Progress
Key Architectural ImprovementsExcellent Design Decisions:
API Design:
Code Quality Issues🔴 Critical Issues1. Unimplemented BrTable ( Operation::BrTable { targets: _ } => {
todo!()
}This will panic at runtime if WASM contains branch tables. Should return a proper error. 2. Type System Inconsistencies (
🟡 Design Concerns1. Error Handling Robustness
2. Code Duplication ( 3. Memory Safety
Test Coverage Assessment✅ Strengths:
Performance ConsiderationsPositive:
Potential Issues:
Security ConsiderationsLow Risk:
Responses to Review Comments@enitrat on generator.rs:197: The removed code appears to be dead - good cleanup. @enitrat on layout.rs: Documentation suggestion is good - the current comment doesn't fully explain the allocation behavior. RecommendationsHigh Priority
Medium Priority
Low Priority
This refactor represents a significant improvement in the WASM compilation pipeline. The core architecture is sound, but addressing the critical issues (especially the |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SolvedBreakTarget::Return(_return_values) => { | ||
| // These seem to never be generated by WOMIR | ||
| Err(DagToCasmError::InvalidControlFlow { | ||
| function_name: self.casm_builder.layout.name.clone(), | ||
| reason: "Conditional return not yet implemented".to_string(), | ||
| operation_context: "resolving BrIf return".to_string(), | ||
| })?; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Operation::BrIfZero(target) => { | ||
| // Inverted conditional branch - condition is the last input | ||
| // BrIfZero takes the branch when condition is ZERO | ||
| let cond_idx = node.inputs.len().checked_sub(1).ok_or_else(|| { | ||
| DagToMirError::InvalidControlFlow { | ||
| function_name: self.mir_function.name.clone(), | ||
| DagToCasmError::InvalidControlFlow { | ||
| function_name: self.casm_builder.layout.name.clone(), | ||
| reason: "BrIfZero without condition input".to_string(), | ||
| operation_context: "resolving BrIfZero condition".to_string(), | ||
| } | ||
| })?; | ||
| let condition_value = self.get_input_value(&node.inputs[cond_idx])?; | ||
| let else_target = self.resolve_break_target(node_idx, node, target)?; | ||
| let then_target = self.mir_function.add_basic_block(); | ||
|
|
||
| // Edge copies on the taken edge | ||
| self.record_edge_values(node, target)?; | ||
| let branch_values = self.get_branch_values(node)?; | ||
|
|
||
| let terminator = Terminator::branch(condition_value, then_target, else_target); | ||
| self.get_current_block()?.set_terminator(terminator); | ||
| self.set_current_block(then_target); | ||
| let resolved_target = self.resolve_break_target(node_idx, node, target)?; | ||
|
|
||
| match resolved_target { | ||
| SolvedBreakTarget::Label(label) => { | ||
| let fallthrough_label = | ||
| self.casm_builder.emit_new_label_name(".fallthrough"); | ||
|
|
||
| // If condition is non-zero, skip the branch (jump to fallthrough) | ||
| self.casm_builder | ||
| .jnz(condition_value, fallthrough_label.as_str())?; | ||
|
|
||
| // Taken path (when zero): store values and jump to target | ||
| self.store_to_label_slots(target, &branch_values)?; | ||
| self.casm_builder.jump(label.as_str()); | ||
|
|
||
| // Fallthrough path continues here | ||
| self.casm_builder.emit_add_label(Label { | ||
| name: fallthrough_label, | ||
| address: None, | ||
| }); | ||
| } | ||
| SolvedBreakTarget::Return(_return_values) => { | ||
| // These seem to never be generated by WOMIR | ||
| Err(DagToCasmError::InvalidControlFlow { | ||
| function_name: self.casm_builder.layout.name.clone(), | ||
| reason: "Conditional return not yet implemented".to_string(), | ||
| operation_context: "resolving BrIfZero return".to_string(), | ||
| })?; |
There was a problem hiding this comment.
The new BrIf and BrIfZero handlers bail out with InvalidControlFlow whenever the resolved break target represents a return. The previous MIR-based lowering handled this case by emitting a conditional branch whose taken edge performed the return. In WebAssembly it is valid (and common) to conditionally return from a function by branching to the outermost block, so this path can occur in real DAGs. With the current code such constructs will cause lowering to fail with "Conditional return not yet implemented", making the compiler reject valid input. Consider emitting the same store-and-return sequence used in the unconditional Br case instead of erroring out.
Useful? React with 👍 / 👎.
This PR refactors the entirety of the WASM crate to emit Cairo-M assembly directly instead of Mir
Changes in codegen API
generator.rsadd_function_from_builderhelper which allows building functions manually without depending on a MirFunction objectlayout.rsbuilder
Changes in wasm crate
cfgops: had to refactor a bit as many opcodes do not have a direct equivalent in CASMOther
main: add DAG printing in --verbose modeCloses CORE-1292