[PATCH] D138203: [AArch64][InstCombine] Simplify repeated complex patterns in dupqlane

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau marked an inline comment as done.
MattDevereau added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1437
 
+static SmallVector<Value *> SimplifyValuePattern(SmallVector<Value *> Vec,
+                                                 bool AllowPoison) {
----------------
sdesmalen wrote:
> It's better to not pass/return large objects by value. If you can't use ArrayRef because you have to modify the values, perhaps you can pass it by reference, and make it both the input and output array (for example by truncating the result)
I'm questioning whether this is adding gold plating. The SmallVector's are of pointers to values so it's just the SmallVector objects that are being copied, and this function will recurse at most 15 times. This will also need rewiring to use bool returns as the input and output vectors from this function are compared for equality to check if any pattern was found. It will be beneficial though so I'll have a go at implementing it.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1483
+    auto Idx = cast<ConstantInt>(InsertElt->getOperand(2));
+    InsertEltVec[Idx->getValue().getZExtValue()] = InsertElt->getOperand(1);
+    CurrentInsertElt = InsertElt->getOperand(0);
----------------
sdesmalen wrote:
> What happens if there is an insert into the same lane?
> 
> e.g. 
> 
>   %1 = insertelement <8 x half> %0, half %x, i64 1
>   %2 = insertelement <8 x half> %1, half %y, i64 1
The latest insert is used (%y in this case) and the transform works ok, I'll add a test for this.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1500-1501
+  for (size_t I = 0; I < Pattern.size(); I++) {
+    if (Pattern[I] == nullptr)
+      continue;
+    Constant *Idx = ConstantInt::get(Builder.getInt64Ty(), APInt(64, I));
----------------
sdesmalen wrote:
> Can this ever happen? I would expect the values in Pattern to always be defined at this point.
It can happen in cases such as <a, b, c, _, a, b, c, _> which I will add a test for


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1503
+    Constant *Idx = ConstantInt::get(Builder.getInt64Ty(), APInt(64, I));
+    if (InsertEltChain == nullptr)
+      InsertEltChain = Builder.CreateInsertElement(
----------------
sdesmalen wrote:
> At this point we know the Pattern.size() > 1, so can you hoist this out of the loop, and start the loop counter `I` at 1 instead of 0?
`Pattern.size() > 1` will be true but Pattern[0] can be null when the pattern `<_, b, c, d>` is evaluated. The previous revision hoisted this, but some tests were breaking as it was trying to create an InsertElement with a null 0th parameter . I think it makes sense to check it in the loop instead of hoisting it to make sure you don't do this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138203