[PATCH] D139339: [InstCombine] Bubble vector.reverse of select operands to their result.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked 4 inline comments as done.
paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2340
+      if (auto *I = dyn_cast<Instruction>(V))
+        I->copyIRFlags(&Sel);
+      Module *M = Sel.getModule();
----------------
david-arm wrote:
> Would it be possible to modify one of the existing tests to have flags so we can correctly defend their propagation? We now have code paths where we depend upon the fast-math flags being set correctly on the select by the loop vectoriser.
I've updated `reverse_select_reverse`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2350
+      if (match(FVal, m_VecReverse(m_Value(Y))) &&
+          (Cond->hasOneUse() || TVal->hasOneUse() || FVal->hasOneUse()))
+        return createSelReverse(C, X, Y);
----------------
sdesmalen wrote:
> what is the reasoning behind //either// of them having a single use?
The only situation where the combine can introduce more reversals than the original input IR is when all operands have more than one use. So if one or more operands have a single use then we can be sure the output will be the same or better.

For the case where the number of reversals is the same the hope is that at some point the reversals will bubble to a point where back-back reversals are seen with both then removed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2354
+      // select rev(C), rev(X), YSplat --> rev(select C, X, FValSplat)
+      if ((Cond->hasOneUse() || TVal->hasOneUse()) && isSplatValue(FVal))
+        return createSelReverse(C, X, FVal);
----------------
david-arm wrote:
> I don't know how far you want to go with this, but you can support more than just splats for the false and true vals. For example, we can also do the transformation when FVal is an arbitrary fixed-width vector constant, i.e.
> 
> ```define <4 x i32> @select_reverse(<4 x i1> %a, <4 x i32> %b, <4 x i32> %c) {
>   %a.rev = tail call <4 x i1> @llvm.experimental.vector.reverse.v4i1(<4 x i1> %a)
>   %b.rev = tail call <4 x i32> @llvm.experimental.vector.reverse.v4i32(<4 x i32> %b)
>   %select = select <4 x i1> %a.rev, <4 x i32> %b.rev, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
>   ret <4 x i32> %select
> }```
`vector.reverse` is not typically used for fixed length vectors as such intrinsics should be converted to `shufflevector` instructions where the existing combines will kick in.

For scalable vectors is there an idiom outside of splats where the operand can be rewritten without introducing an explicit reversal?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139339