[PATCH] D31398: [X86][X86 intrinsics]Folding cmp(sub(a, b), 0) into cmp(a, b) optimization

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2396
+    // Folding cmp(sub(a,b),0) into cmp(a,b)
+    if (Instruction *I = dyn_cast<Instruction>(II->getArgOperand(0))) {
+      if (I->getOpcode() == Instruction::FSub && I->hasOneUse()) {
----------------
You can use 'auto *' with dyn_cast because the type is obvious.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2398-2400
+        // This folding is not valid for safe algebra,
+        // but it doesn't require all of the fast flags
+        // only the ninf flag.
----------------
This comment should specify the non-obvious constraints that we've discussed here:
  // This fold requires NINF because inf minus inf is nan.
  // NSZ is not needed because zeros of any sign are equal for both compares.
  // NNAN is not needed because nans compare the same for both compares.
  // FMF are not needed on the compare intrinsic because...


================
Comment at: test/Transforms/InstCombine/X86FsubCmpCombine.ll:6
+
+define zeroext i16 @fucntionTets(<2 x double> %a, <4 x double> %b, <8 x double> %c, <4 x float> %d, <8 x float> %e, <16 x float> %f,<2 x double> %aa, <4 x double> %bb, <8 x double> %cc, <4 x float> %dd, <8 x float> %ee, <16 x float> %ff) local_unnamed_addr #0 {
+; CHECK-LABEL: @fucntionTets(
----------------
Misspelling in function name. Also, as suggested earlier, I'd really prefer to have one test per intrinsic rather than everything in one function. It makes it a lot easier to see the simple pattern that's getting folded.


https://reviews.llvm.org/D31398