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

[Arch D. Robison via llvm-commits]

ArchDRobison marked 3 inline comments as done.
ArchDRobison added a comment.

Added commentary to line comments by others.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:280
@@ -299,4 +279,3 @@
     ConstantInt *CI = dyn_cast<ConstantInt>(E->getOperand(1));
-
-    if (!CI || CI->getZExtValue() != i || E->getOperand(0) != Vec)
-      return false;
+    return CI && CI->getZExtValue() == i;
+  } else {
----------------
Ayal wrote:
> maybe better to wrap return value inside ( )
My  preference is to avoid superfluous parentheses around return values.  The  style of the source file varies on this point, but in general seems to avoid parentheses around return values of similar complexity.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:283
@@ +282,3 @@
+    assert(Opcode == Instruction::ExtractValue);
+    ExtractValueInst *EI = cast<ExtractValueInst>(E);
+    assert(EI->getNumIndices()==1);
----------------
Ayal wrote:
> cast should be doing the assert above it.
The assert is checking Opcode, not E.  The assert is really intended to check that the parameter Opcode is correct on entry.  To make this clearer, I moved it up to the entry and make it check that Opcode is ExtractElement or ExtractValue.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:284
@@ +283,3 @@
+    ExtractValueInst *EI = cast<ExtractValueInst>(E);
+    assert(EI->getNumIndices()==1);
+    return *EI->idx_begin()==i;
----------------
Ayal wrote:
> hfinkel wrote:
> > You assert this here, but where is it checked?
> Should check similar to the way GEPs are checked to be 2 indexed.
> Also spaces around ==
I'll remove the assert and make the check part of the return value, i.e. 
```
return EI->getNumIndices() == 1 && *EI->idx_begin() == i;
```

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1486
@@ +1485,3 @@
+  }
+  if (!VectorType::isValidElementType(EltT))
+    return 0;
----------------
Ayal wrote:
> better call local isValidElementType to catch FP80, FP128 fwiw.
Thanks!  I hadn't noticed that subtlety.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1502
@@ +1501,3 @@
+
+bool BoUpSLP::canReuseExtract(ArrayRef<Value *> VL, unsigned Opcode) const {
+  assert(Opcode == Instruction::ExtractElement ||
----------------
Ayal wrote:
> may be simpler to obtain Opcode from getSameOpcode(VL), instead of passing it.
I was concerned about not adding more overhead than necessary, though maybe it's not a big deal in practice.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2329
@@ +2328,3 @@
+        return propagateMetadata(V, E->Scalars);
+      }
+    }
----------------
Ayal wrote:
> else ... return Gather? (but w/o explicit 'else')
There are two ways to fix this.  The "canReuseExtract" check could be an assertion, to verify that earlier checking (as it stands in the patch) cancels the scheduling if canReuseExtract returns false.  But it seems better to use Gather as you suggest, in case the check is relaxed in the future.

================
Comment at: test/Transforms/SLPVectorizer/X86/insertvalue.ll:2
@@ +1,3 @@
+; RUN: opt < %s -basicaa -slp-vectorizer -S -mtriple=x86_64-unknown-linux-gnu -mcpu=corei7-avx | FileCheck %s
+
+; CHECK: load <2 x double>
----------------
Fixed by addition of @julia_load_array_of_i16 .


http://reviews.llvm.org/D14185