[PATCH] Optimize scattered vector insert/extract pattern

[Chad Rosier]

I've added a few nits.

Reviewers might be interested to know why you're running the ADCE pass after the SLP pass.

I'll defer to Tim, James, and others to comment on the overall approach.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:64
@@ -63,1 +63,3 @@
 static cl::opt<bool>
+SLPGather("slp-vectorize-gather", cl::ZeroOrMore,
+          cl::init(false), cl::Hidden,
----------------
Please remove the cl::ZeroOrMore option.  These should not be used with cl::opt.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:68
@@ +67,3 @@
+static cl::opt<bool>
+SLPScatter("slp-vectorize-scatter", cl::ZeroOrMore,
+           cl::init(false), cl::Hidden,
----------------
Please remove the cl::ZeroOrMore option.  These should not be used with cl::opt.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:405
@@ -395,2 +404,3 @@
 
+
 private:
----------------
Please do not add white space.

================
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");
----------------
Why is the necessary?

================
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");
----------------
Same.  Is this necessary?

================
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);
----------------
80-column violation?

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

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

================
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];
----------------
Don't evaluate .size() every iteration.

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

================
Comment at: test/Transforms/SLPVectorizer/AArch64/combine-extractelement.ll:5
@@ +4,3 @@
+
+; TO REMOVE THIS COMMENT: this is the first case we want to address: extract
+; all elements of a same  vector (in any order).
----------------
I assume you want to remove this comment along with the others?

================
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
----------------
Shouldn't we be checking something here?

================
Comment at: test/Transforms/SLPVectorizer/AArch64/combine-insertelement.ll:167
@@ +166,3 @@
+
+  %1 = load i8, i8* %arrayidx1
+  %conv1 = zext i8 %1 to i16
----------------
Shouldn't we be checking something here?

================
Comment at: test/Transforms/SLPVectorizer/AArch64/combine-insertelement.ll:179
@@ +178,3 @@
+
+  %1 = load i8, i8* %arrayidx1
+  %conv1 = zext i8 %1 to i16
----------------
Shouldn't we be checking something here?

http://reviews.llvm.org/D9804

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