[PATCH] D16691: [InstCombine] simplify masked load intrinsics with constant masks

[Sanjay Patel via llvm-commits]

spatel added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:690
@@ +689,3 @@
+         "Wrong type for 4th arg");
+
+  Value *LoadPtr = II.getArgOperand(0);
----------------
RKSimon wrote:
> Minor: You can relax the type requirements by using Constant::isNullValue() and Constant::isAllOnesValue()
> 
> But this is true for a lot of the code in InstCombineCalls.cpp .....
Yes, that makes the code simpler.

I think we still want all of the asserts for sanity checking the intrinsic signature, but let me know if I'm missing anything.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:708
@@ +707,3 @@
+  // insertelement into the passthru vector.
+
+  return nullptr;
----------------
RKSimon wrote:
> I'm really not sure whether we should try to do this here or leaving it until lowering. Its making a big assumption that the target is good at scalar loads + insertions.
I wasn't sure about that when I wrote it, so I threw it in to see what the consensus might be.

But I agree with you now. Although I would only expect this intrinsic to be formed from C a intrinsic or the vectorizers, and therefore, implying that the masked op is supported by the target, we shouldn't assume that also means that other vector ops are supported equally. It could also be wrong if someone's trying to minimize size (-Oz). Let's handle other constant values in the DAG.


http://reviews.llvm.org/D16691