[PATCH] D14185: Extend SLP Vectorizer to deal with aggregates

[Michael Zolotukhin via llvm-commits]

mzolotukhin accepted this revision.
mzolotukhin added a comment.
This revision is now accepted and ready to land.

Hi,

The changes look good to me modulo some small remarks, but you might want to wait for other reviewers' "ok" too.
Also, when committing, please check-in clean-ups and refactoring separately from the main part.

Thanks,
Michael


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:281
@@ +280,3 @@
+/// \returns True if Extract{Value,Element} instruction extracts element i.
+static bool matchExtractIndex(Instruction *E, unsigned i, unsigned Opcode) {
+  assert(Opcode == Instruction::ExtractElement ||
----------------
Nitpick: `i` should be capitalized (or should we use another name at all?).

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:355-364
@@ -373,2 +354,12 @@
     CodeMetrics::collectEphemeralValues(F, AC, EphValues);
+    // Use the vector register size specified by the target unless overridden
+    // by a command-line option.
+    // TODO: It would be better to limit the vectorization factor based on
+    //       data type rather than just register size. For example, x86 AVX has
+    //       256-bit registers, but it does not support integer operations
+    //       at that width (that requires AVX2).
+    if (MaxVectorRegSizeOption.getNumOccurrences())
+      MaxVecRegSize = MaxVectorRegSizeOption;
+    else
+      MaxVecRegSize = TTI->getRegisterBitWidth(true);
   }
----------------
I'd suggest committing this refactoring as a separate patch to separate NFC changes from others.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:419
@@ +418,3 @@
+  // \returns maximum vector register size as set by TTI or overridden by cl::opt.
+  unsigned GetMaxVecRegSize() const {
+    return MaxVecRegSize;
----------------
Nitpick: `s/GetMaxVecRegSize/getMaxVecRegSize/`

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1167
@@ -1159,3 +1166,3 @@
     case Instruction::ExtractElement: {
-      bool Reuse = CanReuseExtract(VL);
+      bool Reuse = canReuseExtract(VL, Opcode);
       if (Reuse) {
----------------
This clean-up also should be committed separately.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1500
@@ +1499,3 @@
+  uint64_t VTSize = DL.getTypeSizeInBits(VectorType::get(EltT, N));
+  if (VTSize < MinVecRegSize || VTSize > MaxVecRegSize || VTSize != DL.getTypeSizeInBits(T))
+    return 0;
----------------
Should we use `getTypeSizeInBits`, or `getTypeStoreSizeInBits`? I remember a bug when we used a wrong one.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1504-1508
@@ +1503,7 @@
+    // Check that struct is homogeneous
+    auto I = ST->element_begin();
+    auto E = ST->element_end();
+    while (++I != E)
+      if (*I != EltT)
+        return 0;
+  }
----------------
This probably can be rewritten as a range loop, or even with `std::all_of`?

================
Comment at: test/Transforms/SLPVectorizer/X86/insertvalue.ll:3-6
@@ +2,6 @@
+
+; CHECK: load <2 x double>
+; CHECK: load <2 x double>
+; CHECK: fmul <2 x double>
+; CHECK: fadd <2 x double>
+define void @julia_2xdouble([2 x double]* sret, [2 x double]*, [2 x double]*, [2 x double]*) {
----------------
Please add `CHECK-LABEL` statements before each function. That'll help to avoid possible interference between `CHECK` statements from different tests.


http://reviews.llvm.org/D14185