Skip to content

Conversation

@MaxGraey
Copy link
Contributor

@MaxGraey MaxGraey commented Oct 1, 2020

After #3155 we could add more NaN-sign sensitive simplifications for float operations.
x * -1.0 -> -x (fneg(x))

double value;
if (fastMath &&
matches(curr, binary(Abstract::Sub, any(), fval(&value))) &&
if (matches(curr, binary(Abstract::Sub, any(), fval(&value))) &&
Copy link
Contributor Author

@MaxGraey MaxGraey Oct 1, 2020

Choose a reason for hiding this comment

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

Also removed fastMath guard from here to allow canonicalize x - (-0.0) ==> x + 0.0 which is always safe

Copy link
Member

Choose a reason for hiding this comment

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

@kripken what was the verdict here? I thought we had decided this wasn't safe.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is safe as we do still have an operation - we just replace an add with a subtract, and they should have the same impact on NaN bits. Also, I don't think making the constant negative makes a difference. So this looks ok to me.

I'd recommend verifying this with another data point, like running the wasm before and after this opt in V8, and seeing that indeed all is as expected. That would prevent the fuzzer from finding a problem later - and since it can detect this, if it can't find a problem, I think we're ok.

@MaxGraey
Copy link
Contributor Author

MaxGraey commented Oct 1, 2020

Fuzzed:

Invocations so far:
   FuzzExec: 14258
   CompareVMs: 3885
   CheckDeterminism: 1198
   Wasm2JS: 3382
   Asyncify: 3651

ITERATION: 16749

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

The transformation itself looks good, but I'm not so sure about the other changes.

double value;
if (fastMath &&
matches(curr, binary(Abstract::Sub, any(), fval(&value))) &&
if (matches(curr, binary(Abstract::Sub, any(), fval(&value))) &&
Copy link
Member

Choose a reason for hiding this comment

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

@kripken what was the verdict here? I thought we had decided this wasn't safe.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

@MaxGraey, ping me once you've double checked the safety of x - (-0.0) ==> x + 0.0 on V8 as @kripken suggested and we can merge this 👍

@MaxGraey
Copy link
Contributor Author

MaxGraey commented Oct 5, 2020

This is WebAssembly fiddle which compare different variant of subtractions NaN (quiet, signal, arithm, non-canonical, signed and unsigned) and negative zero with similar but optimized version: https://webassembly.studio/?f=fs4kfeplvgb

On Chrome 85. All transforms valid.
On Firefox 81. All transforms valid as well.

@MaxGraey MaxGraey requested a review from tlively October 5, 2020 20:11
@tlively tlively merged commit fc7f66a into WebAssembly:master Oct 5, 2020
@MaxGraey MaxGraey deleted the opt-fmul-with-neg-one branch October 5, 2020 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants