[PATCH] D100273: [VectorCombine] Scalarize vector load/extract.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/VectorCombine/X86/load-inseltpoison.ll:645
   %gep = getelementptr inbounds <2 x i16>, <2 x i16>* %p, i64 1
   %l = load <2 x i16>, <2 x i16>* %gep, align 8
   %s = extractelement <2 x i16> %l, i32 0
----------------
spatel wrote:
> fhahn wrote:
> > RKSimon wrote:
> > > The arg says its align 1m, but here we're saying align 8?
> > Hm that's indeed odd. I *think* the align of the load would take precedence and `align 1 dereferenceable(16)` are probably not needed for the test. But I think we need to choose a conservative alignment for the scalarized load, that's why `align 1` is used for the scalar load.
> > 
> > Does that answer your question?
> Some of these tests are intentionally bizarre -- blame me :) -- with respect to alignment because the rules are not clear. 
> Looking at the code in vectorizeLoadInsert(), we have:
>   // Use the greater of the alignment on the load or its source pointer.
>   Alignment = std::max(SrcPtr->getPointerAlignment(DL), Alignment);
> 
> So on the AVX2 target, this test is going through 2 transforms now instead of just 1:
> 1. This patch kicks in 1st and converts the short vector load to scalar load with `align 2` -- presumably because that's the datalayout-ABI-specified setting. Should the original load's `align 8` attribute override that?
> 2. The scalar load is converted to the wider vector load again in vectorizeLoadInsert(), but now the alignment was reduced, so we end up with the test diff.
Thanks for the explanation. I think once we lowered to alignment on the load, it is not really safe to raise it based on the alignment of the pointer type. But if we scalarize with index 0, we load from the original pointer and we can preserve the original alignment. I updated the patch and the AVX2 changes are now gone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100273