[PATCH] D32338: InstructionSimplify: Canonicalize shuffle operands. NFC-ish.

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4148
   for (unsigned i = 0; i != MaskNumElts; ++i) {
     int Idx = ShuffleVectorInst::getMaskValue(Mask, i);
     if (Idx == -1)
----------------
RKSimon wrote:
> You are calling ShuffleVectorInst::getMaskValue multiple times per element - better to call ShuffleVectorInst::getShuffleMask instead?
Right, this is the case even without this patch (see lines 4158 and 4188). Will add a parent patch to address that.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4170
+  // Canonicalization: if only one input vector is contant, it shall be the
+  // second one.
+  if (Op0Const && !Op1Const) {
----------------
RKSimon wrote:
> Add tests for this?
The tests are already there and this is supposed to be a NFC patch.
This canonoicalization allowed the removal of lines 4169-4171 which are a duplication of lines 4166-4168 (or lines 4188-4190 after the patch is applied).
One test-case that should give coverage is the function 'splat_operand3' in test/Transforms/InstSimplify/shufflevector.ll


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4197
+    return nullptr;
 
   // Check if every element of this shuffle can be mapped back to the
----------------
RKSimon wrote:
> Add an assert here to check that NewIndices has MaskNumElts elements
Thanks for pointing this out. I didn't really intend for the newly created mask to be illegal in the case it contains an undef. Will fix this.


Repository:
  rL LLVM

https://reviews.llvm.org/D32338