[PATCH] Extend LoopVectorizationLegality::isConsecutivePtr to handle multiple level GEPs

[Wei Mi]

Michael, Thanks for the review.

> could you please clarify, if this patch depends on http://reviews.llvm.org/D9865 or not?


The patch doesn't depend on http://reviews.llvm.org/D9865. The motivation is to enhance the analysis independently, .i.e, even when gep related IR is not in an expected shape, the analysis can still be valid.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1738-1739
@@ -1699,2 +1737,4 @@
   GetElementPtrInst *Gep = dyn_cast<GetElementPtrInst>(Ptr);
-  if (Gep && Legal->isInductionVariable(Gep->getPointerOperand())) {
+  if (Gep && (GepPtr = Gep->getPointerOperand()) &&
+      (GepPtrInst = dyn_cast<Instruction>(GepPtr)) &&
+      !SE->isLoopInvariant(SE->getSCEV(GepPtrInst), OrigLoop)) {
----------------
mzolotukhin wrote:
> Could we use `GepPtrInst = dyn_cast_or_null(Gep->getPointerOperand()` instead of `(GepPtr = Gep->getPointerOperand()) && (GepPtrInst = dyn_cast<Instruction>(GepPtr)`?
Yes, it is better. 

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1740
@@ +1739,3 @@
+      (GepPtrInst = dyn_cast<Instruction>(GepPtr)) &&
+      !SE->isLoopInvariant(SE->getSCEV(GepPtrInst), OrigLoop)) {
+    // The case Gep->getPointerOperand() is an induction variable
----------------
mzolotukhin wrote:
> I don't understand why it's correct. Could you please clarify the logic behind it?
> 
> Originally the condition was true when the pointer operand was an induction variable. Now it can be true for an arbitrary non-invariant expression that happen to have a specific gep-structure.
Originally there are two cases where a load/store is consecutive:
case1. The pointer operand of gep is a phi and it is an induction variable.
case2. The pointer operand is invariant, only one index operand of the gep is a loop induction variable and all the other index operands on the right hand side of the variant index operand are all 0.

The one more case (case 3) added in the patch is when the pointer operand of gep (named as gep_a) is another gep (named as gep_b). For such load/store to be consecutive, all the index operands of gep_a are all 0 , and gep_b should be case 1 or 2 or another recurisive gep.

For both case1 and case3, the pointer operand of the original gep has const stride so it is loop variant. For case2, the pointer operand of the original gep is loop invariant. That is why case3 can reuse the same logic as case1 in InnerLoopVectorizer::vectorizeMemoryInstruction.


================
Comment at: test/Transforms/LoopVectorize/pr23580.ll:47-69
@@ +46,25 @@
+for.body:                                         ; preds = %for.cond13
+  %idxprom15 = sext i32 %k.0 to i64
+  %arrayidx16 = getelementptr inbounds %struct.B, %struct.B* %add.ptr, i64 %idxprom15
+  %ival = getelementptr inbounds %struct.B, %struct.B* %arrayidx16, i32 0, i32 0
+  %tmp9 = load i16, i16* %ival, align 2
+  %conv17 = sext i16 %tmp9 to i32
+  %add = add nsw i32 %k.0, 1
+  %idxprom18 = sext i32 %add to i64
+  %arrayidx19 = getelementptr inbounds %struct.B, %struct.B* %add.ptr, i64 %idxprom18
+  %ival20 = getelementptr inbounds %struct.B, %struct.B* %arrayidx19, i32 0, i32 0
+  %tmp10 = load i16, i16* %ival20, align 2
+  %conv21 = sext i16 %tmp10 to i32
+  %add22 = add nsw i32 %conv17, %conv21
+  %mul = mul nsw i32 %tmp1, %add22
+  %add23 = add nsw i32 %tmp4, %mul
+  %shr = ashr i32 %add23, %tmp4
+  %arrayidx25 = getelementptr inbounds %struct.B, %struct.B* %call, i64 %idxprom15
+  %ival26 = getelementptr inbounds %struct.B, %struct.B* %arrayidx25, i32 0, i32 0
+  %tmp11 = load i16, i16* %ival26, align 2
+  %conv27 = sext i16 %tmp11 to i32
+  %sub = sub nsw i32 %conv27, %shr
+  %conv28 = trunc i32 %sub to i16
+  store i16 %conv28, i16* %ival26, align 2
+  br label %for.cond13
+
----------------
mzolotukhin wrote:
> Why do we need such complicated loop body, if we're basically only interested in gep+gep+load? Also, the control flow in this test is strange, and I'm not sure if it's necessary for the purpose of the test.
> 
> Could we simplify it?
I will simplify the test.

http://reviews.llvm.org/D10281

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