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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1437
 
+static SmallVector<Value *> SimplifyValuePattern(SmallVector<Value *> Vec,
+                                                 bool AllowPoison) {
----------------
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)


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


================
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));
----------------
Can this ever happen? I would expect the values in Pattern to always be defined at this point.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1503
+    Constant *Idx = ConstantInt::get(Builder.getInt64Ty(), APInt(64, I));
+    if (InsertEltChain == nullptr)
+      InsertEltChain = Builder.CreateInsertElement(
----------------
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?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1522
+  IntegerType *WideTy = Builder.getIntNTy(PatternWidth);
+  ScalableVectorType *WideScalableTy =
+      ScalableVectorType::get(WideTy, PatternElementCount);
----------------
nit: you can use `auto` here, because it's obvious from the RHS what the type will be


================
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:
> nit: you can drop the `std::` here.
You marked this as Done, but it seems unchanged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138203