[PATCH] D83779: [SLP] Fix order of `insertelement`/`insertvalue` seed operands

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 18 inline comments as done.
anton-afanasyev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7084
+    return true;
+  } else {
+    auto *IV = cast<InsertValueInst>(InsertInst);
----------------
ABataev wrote:
> No need for `else` here, just unconditional block, since `then` part is terminated with `return`
Thanks, fixed


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7118
+      return true;
+    } else
+      return false;
----------------
ABataev wrote:
> No need for `else` here
Fixed


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7120
+      return false;
+  } else {
+    auto *IV = cast<InsertValueInst>(InsertInst);
----------------
ABataev wrote:
> Remove `else` here
Removed


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7131
+      } else
+        return false;
+      OperandIndex += Index;
----------------
ABataev wrote:
> Enclose in braces
Enclosed


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7161
+
+  if (BuildVectorOpds.size() == 0) {
+    unsigned int AggregateSize;
----------------
ABataev wrote:
> `.empty()`
Changed


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7363
+  if (!findBuildAggregate(IVI, TTI, BuildVectorOpds, BuildVectorInsts, 0) ||
+      (!llvm::all_of(BuildVectorOpds, [](Value *V) { return V != nullptr; })))
     return false;
----------------
ABataev wrote:
> Very strange check, why we need it? Why did you change the previous one?
The way the `findBuildAggregate()` works has changed: it outputs vector of `Value`s, ordered by `insert`s indices, so it possible this vector contains gaps. But I've changed `findBuildAggregate()` again, splitting it to recursive and non-recursive parts, and making this check inside the latter one.


================
Comment at: llvm/test/Transforms/SLPVectorizer/AArch64/gather-root.ll:206-209
+; MAX-COST-NEXT:    [[P0:%.*]] = load i8, i8* getelementptr inbounds ([80 x i8], [80 x i8]* @a, i64 0, i64 1), align 1
+; MAX-COST-NEXT:    [[P1:%.*]] = icmp eq i8 [[P0]], 0
+; MAX-COST-NEXT:    [[P2:%.*]] = load i8, i8* getelementptr inbounds ([80 x i8], [80 x i8]* @a, i64 0, i64 2), align 2
+; MAX-COST-NEXT:    [[P3:%.*]] = icmp eq i8 [[P2]], 0
----------------
ABataev wrote:
> A regression?
Fixed by allowing to `findBuildAggregate()` support of partial vector filling.


================
Comment at: llvm/test/Transforms/SLPVectorizer/X86/alternate-cast.ll:293-296
+; CHECK-NEXT:    [[AB0:%.*]] = sitofp i32 [[A0]] to float
+; CHECK-NEXT:    [[AB1:%.*]] = sitofp i32 [[A1]] to float
+; CHECK-NEXT:    [[AB2:%.*]] = sitofp i32 [[A2]] to float
+; CHECK-NEXT:    [[AB3:%.*]] = sitofp i32 [[A3]] to float
----------------
ABataev wrote:
> Regression?
Fixed


================
Comment at: llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll:129-134
+; ANY-NEXT:    [[REORDER_SHUFFLE:%.*]] = shufflevector <4 x i32> [[C:%.*]], <4 x i32> undef, <4 x i32> <i32 2, i32 1, i32 0, i32 3>
+; ANY-NEXT:    [[REORDER_SHUFFLE1:%.*]] = shufflevector <4 x float> [[A:%.*]], <4 x float> undef, <4 x i32> <i32 2, i32 1, i32 0, i32 3>
+; ANY-NEXT:    [[REORDER_SHUFFLE2:%.*]] = shufflevector <4 x float> [[B:%.*]], <4 x float> undef, <4 x i32> <i32 2, i32 1, i32 0, i32 3>
+; ANY-NEXT:    [[TMP1:%.*]] = icmp ne <4 x i32> [[REORDER_SHUFFLE]], zeroinitializer
+; ANY-NEXT:    [[TMP2:%.*]] = select <4 x i1> [[TMP1]], <4 x float> [[REORDER_SHUFFLE1]], <4 x float> [[REORDER_SHUFFLE2]]
+; ANY-NEXT:    [[TMP3:%.*]] = extractelement <4 x float> [[TMP2]], i32 2
----------------
ABataev wrote:
> Regression?
Yes, that's regression, but that's pay for the optimization by reducing `shufflevector`s at the more common cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83779