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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi,

It looks good in general. Please separate changes for instcombine and SLP and find some remarks from me inline (mostly nitpicks).

Thanks,
Michael


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:842
@@ +841,3 @@
+///
+/// \returns underlyingvalue that was "cast", or nullptr otherwise.
+static Value* likeBitCastFromVector(InstCombiner &IC, Value* V) {
----------------
s/underlyingvalue/underlying value/

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:843
@@ +842,3 @@
+/// \returns underlyingvalue that was "cast", or nullptr otherwise.
+static Value* likeBitCastFromVector(InstCombiner &IC, Value* V) {
+  Value *U = nullptr;
----------------
I'd prefer to have an example in the comment as well.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:850
@@ +849,3 @@
+    auto *W = EI->getVectorOperand();
+    if (!U) {
+      U = W;
----------------
Unnecessary curly braces?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:287
@@ +286,3 @@
+  } 
+  if (N != 2 && N != 4 && N != 8 && N != 16)
+    // Reject other vector lengths as generally unprofitable.
----------------
I think it should depend on available vector register size, i.e. on `MaxVectorRegSizeOption` and `MinVecRegSize`.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4013
@@ +4012,3 @@
+///
+/// \return true if it matches
+static bool findBuildAggregate(InsertValueInst *IV,
----------------
Nitpick: missing dot in the end.

================
Comment at: test/Transforms/SLPVectorizer/X86/insertvalue.ll:1
@@ +1,2 @@
+; RUN: opt < %s -basicaa -slp-vectorizer -S -mtriple=x86_64-unknown-linux-gnu -mcpu=corei7-avx | FileCheck %s
+
----------------
Could you please also add a test checking that `[2 x i2]` isn't vectorized to `<2 x i2>` here? Similar to other tests in `test/Transforms/SLPVectorizer/X86/bad_types.ll`


http://reviews.llvm.org/D14185