[PATCH] D152386: [LoadStoreVectorizer] Only upgrade align for alloca

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:805
+      Value *PtrOperand = getLoadStorePointerOperand(C[CBegin].Inst);
+      bool IsAllocaAccess = isa<AllocaInst>(PtrOperand->stripInBoundsOffsets());
       Align Alignment = getLoadStoreAlignment(C[CBegin].Inst);
----------------
bjope wrote:
> jlebar wrote:
> > getOrEnforceKnownAlignment will merely stripPointerCasts, which is weaker than stripInBoundsOffsets.  Presumably we should do the same here?
> I agree that it makes sense to use the same kind of strip here.
> 
> When using stripPointerCasts I got some more diffs in the `@load_alloca16_unknown_offset_align1_i32` test case, so then I changed this to use the stronger stripInBoundsOffsets to make the impact of my fixup smaller.
> 
> There is however already a FIXME for that specific test:
> ```
> ; FIXME: Although the offset is unknown here, we know it is a multiple
> ; of the element size, so should still be align 4
> ```
> So I think that we will get back to a state when that FIXME makes sense (when the test show an opportunity to improve something since we do not vectorize the ALIGNED case any longer).
Sorry, that FIXME was about `@load_unknown_offset_align1_i32` (so not the same test case).

Anyway, I've changed to stripPointerCasts, and then `@load_alloca16_unknown_offset_align1_i32` was impacted as it no longer vectorize the ALIGNED case. I don't know if that test case is a bit contrived anyway (why would it use `align 1` on those i32 loads in the first place).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152386/new/

https://reviews.llvm.org/D152386