chore(singlepass): simplify arch_supports_x #5917
Conversation
Right now, all targets support canonicalization and the indirect call trampolines are unnecessary.
There was a problem hiding this comment.
Pull request overview
This PR simplifies the singlepass compiler by removing two architecture capability checks that are no longer needed: arch_supports_canonicalize_nan and arch_requires_indirect_call_trampoline. The changes reflect that all currently supported targets support NaN canonicalization and do not require indirect call trampolines.
Key changes:
- Removed
arch_supports_canonicalize_nan()andarch_requires_indirect_call_trampoline()methods from the Machine trait and all implementations - Simplified conditional logic throughout the codebase to always assume these capabilities are available
- Removed conditional branching in f32/f64 min/max operations that previously checked for canonicalization support
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/compiler-singlepass/src/machine.rs | Removed trait method declarations for the two architecture capability checks |
| lib/compiler-singlepass/src/emitter_x64.rs | Removed x64-specific implementations that always returned true/false for the capability checks |
| lib/compiler-singlepass/src/emitter_arm64.rs | Removed ARM64-specific implementations that always returned true/false for the capability checks |
| lib/compiler-singlepass/src/machine_x64.rs | Removed method delegations and simplified f32/f64 min/max functions to always use canonicalization path |
| lib/compiler-singlepass/src/machine_arm64.rs | Removed method delegations and simplified canonicalization usage in memory operations |
| lib/compiler-singlepass/src/codegen.rs | Simplified all conditional checks to only test enable_nan_canonicalization config flag, removed trampoline conditional logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| loc_b: Location, | ||
| ret: Location, | ||
| ) -> Result<(), CompileError> { | ||
| if !self.arch_supports_canonicalize_nan() { |
There was a problem hiding this comment.
This should be more like if we want to use nan canonicalization or not (regardless if the architecture supports it)
There was a problem hiding this comment.
If I do that, I get to the following spec issues even w/o the NaN canonicalization:
Error: Failed directives on tests/wast/spec/f64.wast:
• expected -0.0 (Core(F64(Value(F64 { bits: 9223372036854775808 })))), got F64(0.0) (9223372036854775808) (1620:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (18443366373989023744) (1652:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (9219994337134247936) (1654:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (18443366373989023744) (1656:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (9219994337134247936) (1658:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (18443366373989023744) (1692:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (9219994337134247936) (1694:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (18443366373989023744) (1696:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (9219994337134247936) (1698:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (18443366373989023744) (1732:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (9219994337134247936) (1734:1)
• expected Core(F64(ArithmeticNan)), got F64(NaN) (18443366373989023744) (1736:1)
The similar thing is also done in the ARM64 target.
Right now, all targets support canonicalization and the indirect call trampolines are unnecessary.