[PATCH] D128732: [VectorCombine] Improve shuffle select shuffle-of-shuffles

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:1499-1507
+  auto GetShuffleOperand = [&](Instruction *I, unsigned Op) -> Value * {
+    auto *SV = dyn_cast<ShuffleVectorInst>(I);
+    if (!SV)
+      return I;
+    if (isa<UndefValue>(SV->getOperand(1)))
+      if (auto *SSV = dyn_cast<ShuffleVectorInst>(SV->getOperand(0)))
+        if (InputShuffles.contains(SSV))
----------------
samtebbs wrote:
> This has the same structure as `GetBaseMaskValue`, so I wonder if `GetMaskValue` can use this. If it makes it messier then having both is OK.
Hmm. Good point, but I'm not sure how easy it is for them to share logic, unfortunately. They are each returning quite different values, and the parameters are different between the two methods.


================
Comment at: llvm/test/Transforms/VectorCombine/AArch64/select-shuffle.ll:411
+; CHECK-NEXT:    [[CONV118:%.*]] = and i32 [[TMP90]], 65535
+; CHECK-NEXT:    [[SHR:%.*]] = lshr i32 [[TMP90]], 16
 ; CHECK-NEXT:    [[ADD119:%.*]] = add nuw nsw i32 [[CONV118]], [[SHR]]
----------------
samtebbs wrote:
> Looks like more instructions are added here. Is that fine because it gets lowered to something better or of lower cost?
Yeah - More instructions is better in this case, as the shuffles are each simpler. It can be hard to see where this is an improvement and where it isn't from the tests. In general it can be a little difficult to be very precise, but most of the changes are improvements (and this one especially is much better as the shuffles are a lot easier to materialize).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128732/new/

https://reviews.llvm.org/D128732