[InstSimplify] More fcmp cases when comparing against negative constants.

[paulwalker-arm]

Summary

For known positive non-zero value X:

fcmp uge X, -C => true
fcmp ugt X, -C => true
fcmp une X, -C => true
fcmp oeq X, -C => false
fcmp ole X, -C => false
fcmp olt X, -C => false

Patch by Paul Walker.

[fhahn]

Looks like a straight-forward simplification to me, but it would be good if someone else could have a look too.

[spatel]

The tests should go under tests/Transforms/InstSimplify since this is handled outside of InstCombine.
I went ahead and added variants of the original tests plus a couple of extras with r319041. The extra tests show at least one possible extension to handle other fcmp predicates. A more general extension might involve a range test rather than using CannotBeOrderedLessThanZero()?

lib/Analysis/InstructionSimplify.cpp

3331–3332

We've already handled isNaN above here? Could assert instead of re-checking that condition or just remove it.

3336

Should add a comment for symmetry:
X > 0 or X > -C

[paulwalker-arm]

Broke out the isNegative case into its own block so that more conditions can be handled when it's known the constant is non-zero. Removed my original tests and updated the existing ones to reflect the optimised output.

[spatel]

LGTM.

Please add a 'TODO' comment in the new block of code about extending this. We should have cases for 'UNE' and 'OEQ' predicates too?

[paulwalker-arm]

Good point, I'll add the extra conditions along with the TODO.

[paulwalker-arm]

Extended to cover une and oeq conditions. Added an assert to protect against unintentional refactoring and a TODO for future improvements.

[spatel]

lib/Analysis/InstructionSimplify.cpp

3347

Nit: The assert string would confuse me if we ever hit that because -NaN actually is a (set of) FP value(s).

"Unexpected NaN constant" ?

test/Transforms/InstCombine/cast-int-fcmp-eq-0.ll

342–349 ↗

(On Diff #124734)

Can you move this and the related tests over to the instsimplify file?

Nit: I'd prefer to change these constants to '-1.0' or whatever they are rather than trying to decode hex values. :)

[paulwalker-arm]

test/Transforms/InstCombine/cast-int-fcmp-eq-0.ll

342–349 ↗

(On Diff #124734)

Will do. Are there any objections to me maintaining the same number of tests within InstSimplify/floating-point-compare.ll but replacing some uses of fabs with uitofp?

[spatel]

test/Transforms/InstCombine/cast-int-fcmp-eq-0.ll

342–349 ↗

(On Diff #124734)

Sounds ok to me. As long we're covering all of the predicates and have some vector tests too, that should keep the fold tested well enough. Might want to vary double/float/half as well just to be sure we hit more types.

[paulwalker-arm]

Removed three instcombine tests that are either covered by existing instsimplify tests of have equivalent instsimplify tests provided by this patch.

[spatel]

LGTM.

lib/Analysis/InstructionSimplify.cpp

if (CFP->getValueAPF().isInfinity()) {
         case FCmpInst::FCMP_ULE:
           // All values are unordered with and at most infinity.
           return getTrue(RetTy);
         default:
           break;
         }
       }
     }
-    if (CFP->getValueAPF().isZero()) {
+    if (CFP->getValueAPF().isZero() ||
+        (CFP->getValueAPF().isNegative() && !CFP->getValueAPF().isNaN())) {
       switch (Pred) {
       case FCmpInst::FCMP_UGE:
         if (CannotBeOrderedLessThanZero(LHS, Q.TLI))
           return getTrue(RetTy);
         break;
       case FCmpInst::FCMP_OLT:
-        // X < 0
+        // X < 0 or X < -C
         if (CannotBeOrderedLessThanZero(LHS, Q.TLI))
           return getFalse(RetTy);
         break;
       default:
         break;
       }
     }
   }
 
   // If the comparison is with the result of a select instruction, check whether
   // comparing with either branch of the select always yields the same value.
   if (isa<SelectInst>(LHS) || isa<SelectInst>(RHS))
     if (Value *V = ThreadCmpOverSelect(Pred, LHS, RHS, Q, MaxRecurse))
       return V;
 
   // If the comparison is with the result of a phi instruction, check whether

test/Transforms/InstCombine/fcmp.ll

 define i1 @test19_undef_ordered() nounwind {
 ; CHECK-LABEL: @test19_undef_ordered(
 ; CHECK-NEXT:    ret i1 false
 ;
   %cmp = fcmp oeq float undef, undef
   ret i1 %cmp
 }
 
+define i1 @test20(double %a) nounwind {
+; CHECK-LABEL: @test20(
+; CHECK-NEXT:    ret i1 false
+;
+  %call = call double @llvm.fabs.f64(double %a)
+  %cmp = fcmp olt double %call, -1.000000e+00
+  ret i1 %cmp
+}
+
+define i1 @test21(double %a) nounwind {
+; CHECK-LABEL: @test21(
+; CHECK-NEXT:    ret i1 true
+;
+  %call = call double @llvm.fabs.f64(double %a)
+  %cmp = fcmp uge double %call, -1.000000e+00
+  ret i1 %cmp
+}