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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau marked 2 inline comments as done.
MattDevereau added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1437
 
+static SmallVector<Value *> SimplifyValuePattern(SmallVector<Value *> Vec,
+                                                 bool AllowPoison) {
----------------
MattDevereau wrote:
> 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.
I've re-written this to now use a single vector passed by reference.


================
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);
----------------
MattDevereau wrote:
> 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.
I've added the test `dupq_f16_abcd_pattern_double_insert` to `sve-intrinsic-dupqlane.ll` which asserts this behaviour.


================
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));
----------------
MattDevereau wrote:
> 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
I've added the test `dupq_f16_abcnull_pattern` to `sve-intrinsic-dupqlane.ll` to assert this. 


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1438
+static SmallVector<Value *> SimplifyValuePattern(SmallVector<Value *> Vec) {
+  std::size_t VecSize = Vec.size();
+  if (VecSize < 2 || !isPowerOf2_64(VecSize))
----------------
sdesmalen wrote:
> sdesmalen wrote:
> > nit: you can drop the `std::` here.
> You marked this as Done, but it seems unchanged.
I've fully replaced references to std::size_t with size_t now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138203