Skip to content

winch: respect the enable_nan_canonicalization setting#12939

Open
r-near wants to merge 2 commits intobytecodealliance:mainfrom
r-near:winch-nan-canonicalization
Open

winch: respect the enable_nan_canonicalization setting#12939
r-near wants to merge 2 commits intobytecodealliance:mainfrom
r-near:winch-nan-canonicalization

Conversation

@r-near
Copy link
Copy Markdown
Contributor

@r-near r-near commented Apr 2, 2026

The enable_nan_canonicalization flag already flows through to Winch via the shared Flags, but Winch was ignoring it. This adds a canonicalize_nan method to the Masm trait that, when the flag is set, emits a compare-with-self + conditional branch to replace NaN results with the canonical quiet NaN after each float arithmetic op.

Covered operations: add, sub, mul, div, min, max, sqrt, ceil, floor, trunc, nearest, demote, and promote. Implemented for x64 and aarch64. Includes a scalar wast test (counterpart to simd/canonicalize-nan.wast).

Copilot AI review requested due to automatic review settings April 2, 2026 04:50
@r-near r-near requested review from a team as code owners April 2, 2026 04:50
@r-near r-near requested review from alexcrichton and removed request for a team April 2, 2026 04:50
@r-near r-near marked this pull request as draft April 2, 2026 04:52
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR ensures Winch respects the existing enable_nan_canonicalization shared setting by adding a canonicalize_nan hook to the MacroAssembler trait and invoking it after scalar floating-point operations so NaN results are normalized to the canonical quiet-NaN bit pattern.

Changes:

  • Add MacroAssembler::canonicalize_nan and implement it for x64 and aarch64.
  • Invoke NaN canonicalization after scalar float arithmetic/conversion ops (and after rounding paths in the visitor).
  • Add a scalar WAST test to validate canonical NaN bit patterns.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
winch/codegen/src/visitor.rs Inserts canonicalize_nan after scalar float ops and adds helper for rounding results.
winch/codegen/src/masm.rs Extends the MacroAssembler trait with canonicalize_nan.
winch/codegen/src/isa/x64/masm.rs Implements NaN detection + replacement with canonical NaN on x64.
winch/codegen/src/isa/aarch64/masm.rs Implements NaN detection + replacement with canonical NaN on aarch64; stores shared flags in the masm.
tests/misc_testsuite/canonicalize-nan-scalar.wast Adds scalar coverage for canonical NaN bit patterns and propagation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@r-near r-near force-pushed the winch-nan-canonicalization branch from fc5efb7 to 97d89bb Compare April 2, 2026 04:58
@r-near r-near force-pushed the winch-nan-canonicalization branch from 97d89bb to 0c5e168 Compare April 2, 2026 04:59
@r-near r-near marked this pull request as ready for review April 2, 2026 05:13
@github-actions github-actions bot added the winch Winch issues or pull requests label Apr 2, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Subscribe to Label Action

cc @saulecabrera

Details This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@r-near
Copy link
Copy Markdown
Contributor Author

r-near commented Apr 2, 2026

@cfallin Thanks for reviewing #12940! Would you mind taking a look at this one as well?

@cfallin
Copy link
Copy Markdown
Member

cfallin commented Apr 2, 2026

@r-near we usually either stick with the assignment bot's suggestions (the reason I reviewed your last PR) or, in cases such as this one, tag a sub-area reviewer who knows part of the code best -- in this case @saulecabrera might be best to review Winch-specific work?

@saulecabrera
Copy link
Copy Markdown
Member

I'll review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

winch Winch issues or pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants