Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InstSimplify: fadd (nsz op), +0 incorrectly removed #45123

Closed
nunoplopes opened this issue May 2, 2020 · 4 comments
Closed

InstSimplify: fadd (nsz op), +0 incorrectly removed #45123

nunoplopes opened this issue May 2, 2020 · 4 comments
Labels
bugzilla Issues migrated from bugzilla miscompilation

Comments

@nunoplopes
Copy link
Member

Bugzilla Link 45778
Resolution FIXED
Resolved on May 05, 2020 15:39
Version trunk
OS All
CC @efriedma-quic,@RKSimon,@regehr,@rotateright
Fixed by commit(s) a954b8a

Extended Description

Test: Transforms/InstSimplify/fast-math.ll
Summary: +0 + +0 == +0 and -0 + +0 == +0 in default rounding mode. Hence below when %nsz is +/-0 the function returns +0 only, while the optimized function returns +/-0.

define float @​fold_fadd_cannot_be_neg0_nsz_src_x_0(float %a, float %b) {
%nsz = fmul nsz float %a, %b
%add = fadd float %nsz, 0.000000
ret float %add
}
=>
define float @​fold_fadd_cannot_be_neg0_nsz_src_x_0(float %a, float %b) {
%nsz = fmul nsz float %a, %b
ret float %nsz
}
Transformation doesn't verify!
ERROR: Value mismatch

Example:
float %a = #x01e21080 (0.000000000000?)
float %b = #x00225d01 (0.000000000000?)

Source:
float %nsz = #x80000000 (-0.0)
float %add = #x00000000 (+0.0)

Target:
float %nsz = #x80000000 (-0.0)
Source value: #x00000000 (+0.0)
Target value: #x80000000 (-0.0)

https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=1ede71ade7988ad1&test=Transforms%2FInstSimplify%2Ffast-math.ll

@rotateright
Copy link
Contributor

Should we clarify the behavior of 'nsz'?

From LangRef:
"No Signed Zeros - Allow optimizations to treat the sign of a zero argument or result as insignificant."

If we make that like "nnan" or "ninf":
"Allow optimizations to assume the arguments and result are not -0.0. If an argument is -0.0 or the result would be -0.0, it produces a poison value instead."

I'm not sure what the implications of that change would be.

If we keep the current definition that uses "insignificant", is there a way to say that -0.0 is an alias bit-value for +0.0 in Alive2?

@nunoplopes
Copy link
Member Author

Should we clarify the behavior of 'nsz'?

From LangRef:
"No Signed Zeros - Allow optimizations to treat the sign of a zero argument
or result as insignificant."

If we keep the current definition that uses "insignificant", is there a way
to say that -0.0 is an alias bit-value for +0.0 in Alive2?

LangRef is a bit vague indeed. What Alive2 implements is that if the input or output is -0 or +0, then it is changed to be a non-deterministic choice of +/- 0.
This is the closest I can think of +/- 0 bit patterns being an alias. But that doesn't make the example in this report correct, though.
The problem is that the source returns the value of a non-nsz instruction, while the target returns from an nsz instruction. We can't accept an optimization as correct that changes the bit-pattern of the result unless it has some fast-math flag.
(the example would pass if the fadd was nsz)

If we make that like "nnan" or "ninf":
"Allow optimizations to assume the arguments and result are not -0.0. If an
argument is -0.0 or the result would be -0.0, it produces a poison value
instead."

I'm not sure what the implications of that change would be.

Uhm, not sure we could use that nsz for clang's -ffast-math then. It feels a bit extreme to give poison for e.g. 1.0 * -0.0.

@rotateright
Copy link
Contributor

The problem is that the source returns the value of a non-nsz instruction,
while the target returns from an nsz instruction. We can't accept an
optimization as correct that changes the bit-pattern of the result unless it
has some fast-math flag.
(the example would pass if the fadd was nsz)

Right - just wanted to decide where to make the logic fix. Proposal:
https://reviews.llvm.org/D79422

@rotateright
Copy link
Contributor

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla miscompilation
Projects
None yet
Development

No branches or pull requests

2 participants