[PATCH] Optimize scattered vector insert/extract pattern

[Lawrence Hu]

I need  ADCE to clean up some code left by SLPVectorizer, and ADCE is only run once in the whole compiling life time, so adding one pass is not a bad thing.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3122
@@ +3121,3 @@
+        if (unsigned count = collectInsertElements(BB)) {
+          (void)count;
+          DEBUG(dbgs() << "SLP: Found " << count << " insertelement to combine.\n");
----------------
mcrosier wrote:
> Why is the necessary?
This is to work around compile warning, similar to existing code

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3130
@@ +3129,3 @@
+        if (unsigned count = collectExtractElements(BB)) {
+          (void)count;
+          DEBUG(dbgs() << "SLP: Found " << count << " extractelement to combine.\n");
----------------
mcrosier wrote:
> Same.  Is this necessary?
This is to work around compile warning, similar to existing code

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3212
@@ +3211,3 @@
+  bool combineExtractElementChain(ArrayRef<ExtractElementInst *> Chain, BoUpSLP &R, Value *&NewExt);
+  bool combineExtractElements(ArrayRef<ExtractElementInst *> ExtractElements, BoUpSLP &R, Value *&NewExt);
+  unsigned collectExtractElements(BasicBlock *BB);
----------------
mcrosier wrote:
> 80-column violation?
will fix that, thx

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4176
@@ +4175,3 @@
+    // TODO: use SCEV?
+    if (!IE->getOperand(1)->hasOneUse())
+      return false;
----------------
mcrosier wrote:
> Please add comments for the various cases you're trying to detect and avoid.
Comments are before loop

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4185
@@ +4184,3 @@
+      ExtSrcTy = Ext->getOperand(0)->getType();
+    }
+    else {
----------------
mcrosier wrote:
> Running clang-format might resolve some of the formatting issues.
will do that, thanks.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4201
@@ +4200,3 @@
+  std::map<unsigned, unsigned> IndexOccurrence;
+  for (unsigned I = 0; I < InsertElementsCandidates.size(); ++I) {
+    InsertElementInst *IE = InsertElementsCandidates[I];
----------------
mcrosier wrote:
> Don't evaluate .size() every iteration.
will fix that

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4344
@@ +4343,3 @@
+
+    // Find the vector used by the insertelement instruction
+    // and the instruction that defines it.
----------------
mcrosier wrote:
> Maximize 80-column.
will fix that

================
Comment at: test/Transforms/SLPVectorizer/AArch64/combine-extractelement.ll:129
@@ +128,3 @@
+; CHECK-LABEL: @test3
+  %x = extractelement <8 x i8> %v1, i32 0
+  %conv1 = zext i8 %x to i16
----------------
mcrosier wrote:
> Shouldn't we be checking something here?
This case is for future extension, I can remove that, but it does hurt to keep it here.

================
Comment at: test/Transforms/SLPVectorizer/AArch64/combine-insertelement.ll:167
@@ +166,3 @@
+
+  %1 = load i8, i8* %arrayidx1
+  %conv1 = zext i8 %1 to i16
----------------
mcrosier wrote:
> Shouldn't we be checking something here?
This case is for future extension, I can remove that, but it does hurt to keep it here.

================
Comment at: test/Transforms/SLPVectorizer/AArch64/combine-insertelement.ll:179
@@ +178,3 @@
+
+  %1 = load i8, i8* %arrayidx1
+  %conv1 = zext i8 %1 to i16
----------------
mcrosier wrote:
> Shouldn't we be checking something here?
This case is for future extension, I can remove that, but it does hurt to keep it here.

http://reviews.llvm.org/D9804

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/