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

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:1420-1428
+      else if (M < static_cast<int>(NumElts)) {
+        auto It = find_if(V1, [M](auto A) { return A.second == M; });
+        assert(It != V1.end() && "Expected all entries in Mask");
+        ReconstructMask.push_back(std::distance(V1.begin(), It));
+      } else {
+        auto It = find_if(V2, [M](auto A) { return A.second == M; });
+        assert(It != V2.end() && "Expected all entries in Mask");
----------------
Do you think it would be worth making an anonymous function for this similar behaviour?


================
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))
----------------
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.


================
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]]
----------------
Looks like more instructions are added here. Is that fine because it gets lowered to something better or of lower cost?


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

https://reviews.llvm.org/D128732