[PATCH] D26905: [SLP] Vectorize loads of consecutive memory accesses, accessed in non-consecutive (jumbled) way.

[Shahid via Phabricator via llvm-commits]

ashahid added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2596
+        if (Instruction *I = dyn_cast<Instruction>(shuf))
+          return propagateMetadata(I, VL);
+        return shuf;
----------------
mssimpso wrote:
> ashahid wrote:
> > mssimpso wrote:
> > > I also saw verifier failures where TBAA metadata had been applied to the shuffle, like:
> > > 
> > > ```
> > > TBAA is only for loads, stores and calls!
> > >   %14 = shufflevector <4 x i32> %13, <4 x i32> undef, <4 x i32> <i32 2, i32 3, i32 0, i32 1>, !tbaa !66
> > > ```
> > Ok, will fix it.
> I think you should probably just copy the metadata from the scalar load to the vector load, like:
> 
> ```
> propagateMetadata(LI, E->Scalars);
> return Shuf;
> ```
> 
> 
Yes, TBAA metadata is not for shufflevector and recently verifier added this assert.


================
Comment at: test/Transforms/SLPVectorizer/X86/horizontal-list.ll:12
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* @n, align 4
 ; CHECK-NEXT:    [[MUL:%.*]] = mul nsw i32 [[TMP0]], 3
----------------
RKSimon wrote:
> The changes in this file are from the regeneration script and are just polluting this patch, I've commit this against trunk at rL290969 - please rebase.
Ok


================
Comment at: test/Transforms/SLPVectorizer/X86/reduction_loads.ll:35
+; CHECK-NEXT:    [[TMP4:%.*]] = extractelement <8 x i32> [[BIN_RDX4]], i32 0
+; CHECK-NEXT:    [[ADD_7:%.*]] = add i32 [[TMP4]], [[SUM]]
 ; CHECK-NEXT:    br i1 true, label %for.end, label %for.body
----------------
RKSimon wrote:
> This looks suspicious - why the lonely change from TMP3 to TMP4?
Oh good catch, I will see.


https://reviews.llvm.org/D26905