Page MenuHomePhabricator

[GVN} don't propagate equality comparisons of FP zero (PR22376)
ClosedPublic

Authored by spatel on Jan 29 2015, 9:40 AM.

Details

Summary

In http://reviews.llvm.org/D6911, we allowed GVN to propagate FP equalities to allow some simple value range optimizations. But that introduced a bug when comparing to -0.0 or 0.0: these compare equal even though they are not bitwise identical.

This patch disallows propagating zero constants in equality comparisons. As shown in http://llvm.org/bugs/show_bug.cgi?id=22376, we can miscompile with the current optimization.

I also checked to make sure that NaN comparisons are handled, and they always appear to be correctly optimized by SimplifyInstruction.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 18971.Jan 29 2015, 9:40 AM
spatel retitled this revision from to [GVN} don't propagate equality comparisons of FP zero (PR22376).
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added reviewers: pete, ArchDRobison, hfinkel.
spatel added a subscriber: Unknown Object (MLST).
ArchDRobison edited edge metadata.Jan 29 2015, 10:30 AM

The patch looks right to me. Informally, it's saying zero is not a constant :-)

rnk added a subscriber: rnk.Jan 29 2015, 10:52 AM

Don't we have FP math flags for this? See FastMathFlags::noSignedZeros(). Looks like we don't have those on fcmp instrs, though.

We should consider doing this folding if signed zeros are disabled.

In D7257#115344, @rnk wrote:

Don't we have FP math flags for this? See FastMathFlags::noSignedZeros(). Looks like we don't have those on fcmp instrs, though.

We should consider doing this folding if signed zeros are disabled.

I thought about making this conditional on the IR-level FMF, but as you noted, they don't apply to fcmp (or intrinsics; see http://llvm.org/bugs/show_bug.cgi?id=21290 ). So until we get FMF fixed, we'd have to rely on a function-level attribute (unsafe-fp-math I think; I don't see an NSZ equivalent).

Ok, if I just put a 'ToDo' comment in this patch for this?

hfinkel accepted this revision.Jan 29 2015, 12:13 PM
hfinkel edited edge metadata.
In D7257#115344, @rnk wrote:

Don't we have FP math flags for this? See FastMathFlags::noSignedZeros(). Looks like we don't have those on fcmp instrs, though.

We should consider doing this folding if signed zeros are disabled.

I thought about making this conditional on the IR-level FMF, but as you noted, they don't apply to fcmp (or intrinsics; see http://llvm.org/bugs/show_bug.cgi?id=21290 ). So until we get FMF fixed, we'd have to rely on a function-level attribute (unsafe-fp-math I think; I don't see an NSZ equivalent).

Ok, if I just put a 'ToDo' comment in this patch for this?

Yes, add a FIXME for this. LGTM.

This revision is now accepted and ready to land.Jan 29 2015, 12:13 PM
This revision was automatically updated to reflect the committed changes.

Thanks all. Added a FIXME comment and a test case for the -0.0 + fcmp une half.
Checked in at r227491.