[PATCH] D105053: [INSTCOMBINE] Transform reduction(shuffle V, poison, unique_mask) to reduction(V).

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline for a few minor points.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2010
+    ArrayRef<int> Mask;
+    if (auto *FVTy = dyn_cast<FixedVectorType>(Arg->getType()))
+      if (CanBeReassociated &&
----------------
Nit: we're not using FVTy, so it would be easier to read (less indented) if we just bail out on scalable vectors:
  if (!isa<FixedVectorType>(Arg->getType())) break;

You could also combine the next set of conditions with this to early exit and remove another level of indent.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2015
+        int Sz = Mask.size();
+        SmallBitVector UsedIndeces(Sz);
+        for (int Idx : Mask) {
----------------
Unusual spelling: Indices or Indexes?


================
Comment at: llvm/test/Transforms/InstCombine/reduction-shufflevector.ll:90
   %shuf = shufflevector <4 x float> %x, <4 x float> poison, <4 x i32> <i32 2, i32 0, i32 3, i32 1>
   %res = call float @llvm.vector.reduce.fmax.v4f32(<4 x float> %shuf)
   ret float %res
----------------
This is correct (no FMF required) based on LangRef, but it would be good to add some extra flags on at least one FP test, so we verify that those are all propagated through the transform.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105053