Issue 311 - Add a pass to make NaN bits deterministic. by data-pup · Pull Request #322 · bytecodealliance/cranelift

data-pup · 2018-05-02T20:51:34Z

This PR intends to fix #311. I have marked some lines that I was unsure about with 'FIXUP' comments, which I would not mind fixing if you have opinions about them. Otherwise, I can simply remove the comments and squash the commits down :)

This passes all of the existing test cases in the codegen crate, as well as the top-level crate. I can add more tests if you think that is warranted, but might need some guidance on that front.

Thanks again to @sunfishcode for the guidance, I had a lot of fun doing this!

data-pup · 2018-05-02T21:14:06Z

I can take care of the linting issues, pardon that. If there are any issues you see regarding the code overall though, let me know!

sunfishcode · 2018-05-02T21:13:36Z

lib/codegen/src/context.rs

        if isa.flags().opt_level() != OptLevel::Fastest {
            self.preopt(isa)?;
        }
+        self.canonicalize_nans(isa)?; // FIXUP: Should this be wrapped in a conditional?


Yes, this should be guarded by if isa.flags().enable_nan_canonicalization, so that we don't do the canonicalization unless we're asked to.

sunfishcode · 2018-05-02T21:28:12Z

lib/codegen/src/nan_canonicalization.rs

+// Canonical 32-bit and 64-bit NaN values.
+static CANON_32BIT_NAN: u32 = 0b01111111100000000000000000000001;
+static CANON_64BIT_NAN: u64 =
+    0b0111111111110000000000000000000000000000000000000000000000000001;


We need to set the most significant bit of the significand field to 1, so that these are quiet NaNs rather than signaling NaNs. (For completeness, I'm ignoring pre-NAN2008 MIPS here.) Even though IEEE 754 doesn't say much here, it does say that a NaN returned from arithmetic is a quiet NaN.

It would also be good to set the least significant bit to 0, so that we're consistent with common hardware implementations such as ARM's "default NaN" mode.

sunfishcode · 2018-05-02T21:30:28Z

lib/codegen/src/nan_canonicalization.rs

+/// `fcopysign` that only operate on the sign bit of a floating point value.
+fn is_fp_arith(pos: &mut FuncCursor, inst: Inst) -> bool {
+    // FIXUP: Should `ceil`, `floor`, `trunc`, `nearest`, and
+    // immediate constants be considered as well?


Yes for ceil, floor, trunc, and nearest, and no for immediate constants.

Awesome, can do :)

sunfishcode · 2018-05-02T21:34:00Z

lib/codegen/meta/base/settings.py

+        """
+        Enable NaN canonicalization
+        """,
+        default=True)


I think the default should be False, since wasm doesn't require this behavior.

sunfishcode · 2018-05-02T21:35:37Z

lib/codegen/meta/base/settings.py


+enable_nan_canonicalization = BoolSetting(
+        """
+        Enable NaN canonicalization


It'd be good to briefly explain what canonicalization means here, and mention the word "deterministic", to help people who need that find it.

data-pup · 2018-05-02T21:41:54Z

Thanks for the review by the way! I'll add those changes, the commit that was just pushed is fixing the formatting problems. I'll take care of the other points you mentioned tonight.

sunfishcode · 2018-05-02T21:41:16Z

lib/codegen/src/nan_canonicalization.rs

+/// identify and replace NaN's with a single canonical NaN value.
+fn add_nan_canon_instrs(pos: &mut FuncCursor, inst: Inst) {
+    // FIXUP: Are these unwraps safe? An arithmetic operation should never be
+    // found at the end of an EBB, if I understand this correctly.


Yeah, the unwraps are safe, though this would be a place where using .expect("...") would be a little friendlier than .unwrap() because forgetting to add a terminator to an ebb is a bug that does sometimes happen.

sunfishcode · 2018-05-02T21:44:40Z

lib/codegen/src/timing.rs

    binemit: "Binary machine code emission",
    layout_renumber: "Layout full renumbering",
+
+    canonicalize_nans: "Canonicalization of NaN's",


Style nit: no apostrophe. "NaNs" is the plural of "NaN".

sunfishcode · 2018-05-02T21:48:04Z

lib/codegen/src/nan_canonicalization.rs

+
+/// Given some instruction that could potentially return a nondeterministic
+/// NaN value, determine if the operation is using 32-bit or 64-bit floating
+/// point numbers, and return the corresponding NaN value.


You can get this type from the instruction's result type, rather than digging into the operands. func.dfg.value_type(func.dfg.first_result(inst)) or so. Which is simpler, but also, that's the value that you'll be canonicalizing.

sunfishcode · 2018-05-02T23:29:37Z

lib/codegen/src/nan_canonicalization.rs

+    pos.goto_inst(next_inst);
+
+    // Insert a select instruction to canonicalize the NaN value.
+    pos.ins().select(is_nan, canon_nan_res, inst_res);


Ok, so now let's talk about wiring in the new result value :-). The tricky thing is that we don't want to rewire all uses of the old value, because we need the fcmp and select to continue to use the old value.

As I mentioned earlier, there are a few ways to do it, but here's what I think is best: You can use dfg's replace_result to replace the result value of the arithmetic instruction with a new Value. Then you can use pos.ins()'s with_result to when inserting the select using the original Value.

data-pup · 2018-05-02T23:33:44Z

Thanks! Wrapping up edits for the previous review notes right now, I will take a look into wiring in the new result once I am done with that :) if I have any questions I’ll follow up with you here or on irc

data-pup · 2018-05-03T13:48:53Z

Took a swing at this, having a little bit of trouble figuring out why the CI build is failing. When I run the test-all.sh script locally, everything seems to work. I've installed flake8 and mypy with pip3, but the check script in lib/codegen/meta does not seem to find them successfully. Aside from that though, I didn't alter any of the files that seem to be causing the build to fail? Not sure how to proceed from here with regards to that :(

I also wanted to check that I had wired up the replace_result and with_result parameters correctly, I was a little worried I might have these mixed up.

Update: Just saw #323, disregard the first paragraph regarding the flake8/mypy questions.

sunfishcode · 2018-05-03T14:20:49Z

lib/codegen/src/nan_canonicalization.rs

+    // the canonical NaN value if `val` is NaN, assign the result to `inst`.
+    let is_nan = pos.ins().fcmp(FloatCC::NotEqual, val, val);
+    let canon_nan = insert_nan_const(pos, val_type);
+    pos.ins().with_result(new_res).select(


This looks right, except that replace_result goes the other way. new_res is the new result value of inst, and val remains the value that inst's users are using.

Ah, thanks. So, fcmp and select should use new_res as a parameter, and I should pass val to with_result? I can do that right now.

sunfishcode

This PR looks almost ready to merge! Just one detail below.

sunfishcode · 2018-05-09T18:21:38Z

lib/codegen/src/nan_canonicalization.rs

+
+// Canonical 32-bit and 64-bit NaN values.
+static CANON_32BIT_NAN: u32 = 0b01111111100000000000000000000001;
+static CANON_64BIT_NAN: u64 = 0b0111111111110000000000000000000000000000000000000000000000000001;


In case github swallowed my comment here, we need to use quiet NaN bit-patterns here, so the most significant bit of the significand field needs to be 1. And then, we should make the last significant bit 0, because that matches what several other systems consider to be the canonical NaN bit pattern.

data-pup · 2018-05-09T18:31:48Z

Neat! I can apply that change today, thank you for the help :)

sunfishcode · 2018-05-09T20:11:50Z

Looks good!

data-pup force-pushed the add-canonicalize-nans-pass-pr branch from 05a2594 to 964640a Compare May 2, 2018 21:38

sunfishcode reviewed May 2, 2018

View reviewed changes

data-pup force-pushed the add-canonicalize-nans-pass-pr branch from 80c5d5b to 86a7b28 Compare May 3, 2018 13:24

sunfishcode reviewed May 3, 2018

View reviewed changes

data-pup force-pushed the add-canonicalize-nans-pass-pr branch from 86a7b28 to 7513d87 Compare May 3, 2018 14:27

sunfishcode reviewed May 9, 2018

View reviewed changes

Issue 311 - Add a pass to make NaN bits deterministic.

2bc6622

data-pup force-pushed the add-canonicalize-nans-pass-pr branch from 7513d87 to 2bc6622 Compare May 9, 2018 18:57

sunfishcode merged commit e7ada03 into bytecodealliance:master May 9, 2018

alexcrichton mentioned this pull request Mar 19, 2024

Cranelift: inconsistent NaN signedness behavior for f64.copysign + f32.demote_f64 bytecodealliance/wasmtime#8179

Closed

Conversation

data-pup commented May 2, 2018

Uh oh!

data-pup commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

data-pup commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

data-pup commented May 2, 2018

Uh oh!

data-pup commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunfishcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

data-pup commented May 9, 2018

Uh oh!

sunfishcode commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

data-pup commented May 3, 2018 •

edited

Loading