[PATCH] D51942: [InstCombine] Fold (C/x)>0 into x>0 if possible

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

The patch looks logically correct, but see inline comments for cosmetic/procedural changes.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:5276-5291
+        // Assume C != 0 is a constant and a and d are floating point variables.
+        // 1:   a != 0       ... Because a is the nominator of a division
+        //                       this is implicitly given by the flag 'ninf'
+        // 2:   d = C / a
+        // 3:   (d < 0)
+        //
+        // To simplify 3: execute the following steps
----------------
This code comment is more confusing than helpful to me.
Could we say something like this instead:
  // When C is not 0.0 and infinities are not allowed:
  // (C / X) < 0.0 is a sign-bit test of X
  // (C / X) < 0.0 --> X < 0.0 (if C is positive)
  // (C / X) < 0.0 --> X > 0.0 (if C is negative, swap the predicate)


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:5323
+        // Finally emit the new fcmp.
+        auto *a = LHSI->getOperand(1);
+        auto NewFCI = new FCmpInst(NewPred, a, RHSC);
----------------
Prefer to use the actual type here rather than auto and fix capitalization: Value *A.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:5324
+        auto *a = LHSI->getOperand(1);
+        auto NewFCI = new FCmpInst(NewPred, a, RHSC);
+        NewFCI->setFastMathFlags(I.getFastMathFlags());
----------------
I think "auto *" or "FCmpInst *" is preferred:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: test/Transforms/InstCombine/fcmp.ll:385
+;   %3 = fcmp ninf olt double %1, 0.0
+define i1 @test20_recip(double %arg_d, float %arg_f) {
+; CHECK-LABEL: @test20_recip(
----------------
I'd prefer to continue with the style of tests as seen above here because that makes it clear exactly what is changing from test to test without extra stuff like xors. So:
1. Make each fdiv/fcmp pair its own function.
2. Auto-generate the complete check lines using the script mentioned in line 1 of this file.

Also:
3. Commit the tests with baseline CHECK lines before this patch, and rebase this patch so we just see the diffs.


https://reviews.llvm.org/D51942