Page MenuHomePhabricator

InstCombine: fix fold "fcmp x, undef" to account for NaN
ClosedPublic

Authored by mehdi_amini on Feb 13 2015, 9:55 AM.

Details

Summary

See the two test cases.

; Can fold fcmp with undef on one side by choosing NaN for the undef

; Can fold fcmp with undef on both side
; fcmp u_pred undef, undef -> true
; fcmp o_pred undef, undef -> false
; because whatever you choose for the first undef
; you can choose NaN for the other undef

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to InstCombine: cannot fold "fcmp x, undef" because x can be NaN.
mehdi_amini updated this object.
mehdi_amini edited the test plan for this revision. (Show Details)
mehdi_amini added reviewers: chandlerc, hfinkel.
mehdi_amini added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.Feb 16 2015, 2:25 AM

I wonder if we could access the target machine specific info to know
if NaN are disabled?
Or what is the right way of getting the -enable-no-nans-fp-math in opt?

Check the fast-math flags on the the operand (call Op->getFastMathFlags()). The LangRef says:

LLVM IR floating-point binary ops (fadd, fsub, fmul, fdiv, frem) have the following flags that can set to enable otherwise unsafe floating point operations
...
nnan - No NaNs - Allow optimizations to assume the arguments and result are not NaN. Such optimizations are required to retain defined behavior over NaNs, but the value of the result is undefined.
...
fast - Fast - Allow algebraically equivalent transformations that may dramatically change results in floating point (e.g. reassociate). This flag implies all the others.

For constant values, you can call C->isNaN();

mehdi_amini edited edge metadata.

After more thought, I found out we can fold depending on if fcmp is or isn't ordered.

mehdi_amini retitled this revision from InstCombine: cannot fold "fcmp x, undef" because x can be NaN to InstCombine: fix fold "fcmp x, undef" to accound for NaN .Feb 21 2015, 1:07 PM
mehdi_amini updated this object.
mehdi_amini retitled this revision from InstCombine: fix fold "fcmp x, undef" to accound for NaN to InstCombine: fix fold "fcmp x, undef" to account for NaN .Feb 25 2015, 10:00 PM

Ping.

Ping. Can someone look at this InstCombine change?

chandlerc edited edge metadata.Mar 2 2015, 9:15 AM

CC-ing David who seems like he might be in a good position to review this.

majnemer added inline comments.
lib/Analysis/InstructionSimplify.cpp
3058

It would be nice to have LHS on the left and RHS on the right.

I think the comment would be clearer if it mentioned that this simplification will effect fcmp pred undef, x as well.

3059

Isn't Pred already a CmpInst::Predicate? What does this line achieve?

3061

s/fails/fail/

lib/IR/ConstantFold.cpp
1330

perhaps "use the standard constant folder."

1674–1676

Either move the ICmpInst::isIntPredicate(Predicate) && isa<UndefValue>(C1) && isa<UndefValue>(C2) check out of this block or simplify it with ICmpInst::isIntPredicate(Predicate) && C1 == C2, the line is getting a bit too long.

mehdi_amini updated this revision to Diff 21074.Mar 2 2015, 7:17 PM
mehdi_amini retitled this revision from InstCombine: fix fold "fcmp x, undef" to account for NaN to InstCombine: fix fold "fcmp x, undef" to account for NaN.
mehdi_amini edited edge metadata.

Update thanks to David's review.

majnemer accepted this revision.Mar 4 2015, 11:06 PM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Mar 4 2015, 11:06 PM
mehdi_amini closed this revision.Mar 8 2015, 8:22 PM