[PATCH] D31396: [X86][LLVM][Canonical Compare Intrinsics] Creating a canonical representation for X86 CMP intrinsics

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1563
+// Example(a == b) -> (b == a)
+// Target must have VEX encoding!!!
+
----------------
How are you ensuring the VEX encoding is valid?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1569
+  if (isa<Constant>(LHS) && !isa<Constant>(RHS)) {
+    ConstantInt *ComparisonValue = cast<ConstantInt>(II->getOperand(2));
+    uint64_t ConstantValue = ComparisonValue->getZExtValue();
----------------
Unfortunately, there's nothing in the backend IR parsing that guarantees that only a constant for the last intrinsic argument can get here. If you write a bad IR file you can fail this cast. Use a dyn_cast and check it defensively. A bad intrinsic will fail isel later and throw a graceful error, but a bad value here would just cause a crash.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1592
+      const APInt NewComparison(32, (ConstantValue ^ 0xf));
+      II->setOperand(2, ConstantExpr::getIntegerValue(
+                           Type::getInt32Ty(II->getContext()), NewComparison));
----------------
Why are you using ConstantExpr::getIntegerValue? You should be able to use ConstantInt::get right?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2392
+  case Intrinsic::x86_avx512_mask_cmp_ps_256:
+  case Intrinsic::x86_avx512_mask_cmp_ps_512:
+    if(X86CreateCanonicalCMP(II))
----------------
What about sse_cmp_ps, sse2_cmp_pd and their avx2 equivalents?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2505
   // X86 scalar intrinsics simplified with SimplifyDemandedVectorElts.
+  case Intrinsic::x86_sse2_cmp_sd:
+  case Intrinsic::x86_sse_cmp_ss:
----------------
These don't use an i32 for the comparison type immediate, but the X86CreateCanonicalCMP assumes they do when it creates the new Constant.


================
Comment at: test/Transforms/InstCombine/X86CanonicCmp.ll:1
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
----------------
Add test cases for sse_cmp_ss and sse2_cmp_sd.


https://reviews.llvm.org/D31396