LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 45778 - InstSimplify: fadd (nsz op), +0 incorrectly removed
Summary: InstSimplify: fadd (nsz op), +0 incorrectly removed
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Scalar Optimizations (show other bugs)
Version: trunk
Hardware: All All
: P normal
Assignee: Unassigned LLVM Bugs
URL:
Keywords: miscompilation
Depends on:
Blocks:
 
Reported: 2020-05-02 04:08 PDT by Nuno Lopes
Modified: 2020-05-05 15:39 PDT (History)
5 users (show)

See Also:
Fixed By Commit(s): a954b8a363ad


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nuno Lopes 2020-05-02 04:08:15 PDT
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
Comment 1 Sanjay Patel 2020-05-05 06:50:45 PDT
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?
Comment 2 Nuno Lopes 2020-05-05 07:04:14 PDT
(In reply to Sanjay Patel from comment #1)
> 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`.
Comment 3 Sanjay Patel 2020-05-05 08:11:10 PDT
(In reply to Nuno Lopes from comment #2)
> (In reply to Sanjay Patel from comment #1)
> 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
Comment 4 Sanjay Patel 2020-05-05 15:39:51 PDT
https://reviews.llvm.org/rGa954b8a363ad