[PATCH] D149893: Rewrite LSV to handle longer chains.

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:805
+      // existing tests.  This isn't *so* bad, because at most we align to 4
+      // bytes (current value of StackAdjustedAlignment).
+      //
----------------
arsenm wrote:
> jlebar wrote:
> > barannikov88 wrote:
> > > Does DL.getStackAlign() instead of 4 cause many negative differences in tests?
> > > 
> > DL.getStackAlignment() assert-fails on x86, nvptx, and amdgpu:
> > 
> > llvm::DataLayout::getStackAlignment() const: StackNaturalAlign && "StackNaturalAlign must be defined"
> > 
> > Not sure what's going on, but I don't think this is a windmill I want to tilt at in this patch.
> For AMDGPU we definitely do not want to promote stack objects above 4 alignment. We should in fact have optmizations to under-align any stack values to 4 as long as the address isn't captured
> For AMDGPU we definitely do not want to promote stack objects above 4 alignment.

AMDGPU has "S32" in the datalayout string, so this promotion shouldn't happen.

> DL.getStackAlignment() assert-fails on x86, nvptx, and amdgpu:

This is probably because the datalayout in these tests does not have this "S" specifier.
Anyway, it was merely a suggestion, not a request for a change.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149893