Skip x - 0.0 -> x transform if x is non-canonical NaN

[MaxGraey]

Fix #3138

[kripken]

This only works if the left side is a constant seen at compile time. What if it's stored to a local and then read from it? I think the same problem can happen.

Perhaps this needs to be fixed like #3096, that is, by special-casing add/subtract in the interpreter?

[MaxGraey]

Yes -nan * 1 or -nan - 0 -> -nan have similar problem but spec required such operations should produce canonical nans. I guess it better solve on interpreter level which will be used in constant folding as well right? It simplify also this check

[binji]

The NaN does not have to be canonicalized. The NaN payload is allowed to be propagated if it is an arithmetic NaN (i.e. one where the top bit of the payload is set, AKA a "quiet" NaN).

[MaxGraey]

he NaN does not have to be canonicalized. The NaN payload is allowed to be propagated if it is an arithmetic NaN (i.e. one where the top bit of the payload is set, AKA a "quiet" NaN).

Yes, but as I know only canonical qNaN or sNaN should be propagated, other non-canonical nans for example (-NaN) should canonicalized after some arithmetic operations with such nan. Also I found this qNaN * sNaN produce nan:0x200000 (f32) or nan:0x40000000000 (f64).

[binji]

other non-canonical nans for example (-NaN) should canonicalized after some arithmetic operations with such nan

That's allowed, but not required by the spec. The only requirement is that if all NaN inputs to an arithmetic floating-point operation are canonical, then the result will be canonical too. Otherwise the NaN result will be arithmetic (i.e. some quiet NaN).

[MaxGraey]

Otherwise the NaN result will be arithmetic

Yes, that's what I see.

That's allowed, but not required by the spec

Hmm, I thought this spec test for example:

(assert_return_canonical_nan (invoke "add" (f32.const -nan) (f32.const 0x1p-126)))

forces canonicalization due to -nan is not canonical:

binaryen/test/spec/f32.wast

Line 357 in 24d2749

(assert_return_canonical_nan (invoke "add" (f32.const -nan) (f32.const 0x1p-126)))

Or I misunderstand which form of NaN canonical and -nan also canonical?

[MaxGraey]

According to standart spec interpreter assert_return_canonical_nan compare with positive NaN on bit level. However it seems -nan allowed but not -nan:0x326455 for example. So it still required canonization to pure nan or -nan. Which looks like little bit strange due to spec disallow x * -1 -> -x transform due to it may change NaN sign bit, hmm

[binji]

Or I misunderstand which form of NaN canonical and -nan also canonical?

Yes, -nan is canonical, the sign bit is always non-deterministic, unless using abs, neg, copysign or reinterpret operations.

[MaxGraey]

Good to know. Thanks for info!

So according spec we can't propagate -nan:0x134567 - 0.0 or -nan:0x134567 * 1.0 as -nan:0x134567. It should be canonical -nan or +nan as a result of such operations if both are constants

[binji]

So according spec we can't propagate -nan:0x134567 - 0.0 or -nan:0x134567 * 1.0 as -nan:0x134567. It should be canonical -nan or +nan as a result of such operations.

Right, but that's only because those are not arithmetic NaNs. If they were, it would be valid (though not required) to do so, e.g.

;; assuming f32
-nan:0x400001 * 1.0 => -nan:0x400001

is a valid transformation.

EDIT: actually, in your example the result isn't required to be a canonical NaN, it is allowed to be any arithmetic NaN.

[MaxGraey]

-nan:0x400001 * 1.0 => -nan:0x400001
is a valid transformation.

But not for example:
-nan:0x232443 * 1.0 => -nan:0x232443

So if it canonical ((+|-)nan/(+|-)nan:0x800000) or arithmetical NaN ((+|-)nan:0x200000) it propagate "as is", but if it's not canonical nan like (-nan:0x232443) it should canonicalize during constant folding. Right?

[binji]

Yep, but I think it's easier to think of it like this. Extract the NaN payload: if it is arithmetic (which includes canonical), then it can be folded to the original value, otherwise it should probably be folded to a canonical NaN.

That said, it sounds like from @kripken that there are additional constraints that binaryen would like to keep, that may preclude making this optimization.

[kripken]

Sorry if I wasn't clear - IIUC, I think we can do the optimization. We just need to update the interpreter as with #3096 (it's the same issue as there, isn't it? If not I'm missing something).

[MaxGraey]

Yes, just I'm wondering if we not just propagate lhs or rhs but also check NaN's kind in interpreter and canonicalize its if this needs by spec. It will simplify many things in OptimizeInstructions.

[kripken]

@MaxGraey Let's leave the spec discussion as a separate issue? I think we should fix this first, in the interpreter like the similar cases fixed already.

[MaxGraey]

After ~40000 iterations in current upstream fuzzer found problem which relate to same commit with x - 0.0 reduction but this workaround not working any more for this new case. seed number: 2991829000798013883

Reduced example:

(module
  (type $t0 (func))
  (type $t1 (func (param f32)))
  (type $t2 (func (result f64)))
  (type $t3 (func (param i32 i32 v128) (result f64)))
  (import "fuzzing-support" "log-f32" (func $fuzzing-support.log-f32 (type $t1)))
  (func $func_130 (export "func_130") (type $t3) (param $p0 i32) (param $p1 i32) (param $p2 v128) (result f64)
    (i64.store16
      (i32.const 10)
      (i64.const -90))
    (f64.const 0x1.e17p+12 (;=7703;)))
  (func $f2 (type $t2) (result f64)
    (local $l0 f32)
    (call $fuzzing-support.log-f32
      (f32.sub
        (f32.load align=2
          (i32.const 8))
        (local.get $l0)))
    (f64.const 0x0p+0 (;=0;)))
  (func $func_154_invoker (export "func_154_invoker") (type $t0)
    (drop
      (call $f2)))
  (memory $M0 1 1 shared))

and IR before fail:

[f32] (f32.sub
 [f32] (f32.load offset=4 align=2
  [i32] (i32.and
   [i32] (global.get $global$1)
   [i32] (i32.const 15)
  )
 )
 [f32] (f32.const 0)
)

[MaxGraey]

That's look pretty strange for me:

[f32] (f32.sub
 [f32] (f32.load offset=4 align=2
  [i32] (i32.and
   [i32] (global.get $global$1)
   [i32] (i32.const 15)
  )
 )
 [f32] (f32.const 0)
)

@kripken Why we can't do x - 0.0 ==> x reduction when x has side effect?

[kripken]

@MaxGraey

It's not a side effect issue, I don't think? Isn't it what I was saying before?

I think the issue is the same as with #3096 . We need to update the interpreter to not change NaN bits on an add or subtract of 0. Once we do that, the interpreter will not confuse the fuzzer.

That is, the problem is that if we optimize const - 0.0 to const then if the interpreter does something different than keep the NaN bits, it will look like a bug. It isn't a bug because wasm allows various bit patterns for the NaN in that case! But as the fuzzer checks things strictly, we just need to make the interpreter do the obvious thing of not changing NaN bits - which is what the optimization of const - 0.0 to const does. The interpreter must match the optimization, in other words, of all the valid outputs, we should make it emit the one the optimizer can recognize.

And this is a problem in your last testcase even though it has (some complex thing) - 0.0. If "some complex thing" evaluates to a NaN at runtime, the same problem happens - the optimization to remove the - 0.0 leaves the NaN bits of that thing alone. So the interpreter must do the same.

This is a subtle point, and maybe I'm not explaining myself clearly enough, if you don't see my point yet. I can look into fixing this like #3096 if you prefer.

[MaxGraey]

And this is a problem in your last testcase even though it has (some complex thing) - 0.0. If "some complex thing" evaluates to a NaN at runtime, the same problem happens - the optimization to remove the - 0.0 leaves the NaN bits of that thing alone. So the interpreter must do the same.

Oh, that's make sense. Thanks for explanation!

I can look into fixing this like #3096 if you prefer.

That's will awesome! I'm not familiar with interpreter part do better if you fix it here by yourself)

[MaxGraey]

Сlosing in favour #3143