[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 a comment.

What is the plan for supporting the SSE intrinsics?

What does this canonicalization enable if we can't properly do it for the SSE intrinsics? Are we getting worse codegen for the scalar and 128-bit intrinsics on AVX targets just because we can't know we're an AVX target in InstCombine?



================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1570
+  Value *RHS = II->getOperand(1);
+  StringRef IntrinsicName = II->getCalledFunction()->getName();
+  // This Assertion ensures that only avx and above intrinsics are passing
----------------
IntrinsicName is unused in release builds and will throw a warning. Probably need to wrap it in #ifndef NDEBUG


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1578
+            && "Operand must defined by int32 type" );
+    ConstantInt *ComparisonValue = dyn_cast<ConstantInt>(II->getOperand(2));
+    uint64_t ConstantValue = ComparisonValue->getZExtValue();
----------------
You need to check for nullptr have a dyn_cast.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2402
+  case Intrinsic::x86_avx512_mask_cmp_ps_256:
+  case Intrinsic::x86_avx512_mask_cmp_ps_512:
+    if(X86CreateCanonicalCMP(II))
----------------
What about x86_avx_cmp_ps_256 and x86_avx_cmp_pd_256?


https://reviews.llvm.org/D31396