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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:888
+        FixedVT->getElementType(), GEP, EI->getName() + ".scalar"));
+    NewLoad->setAlignment(Align(1));
+    replaceValue(*EI, *NewLoad);
----------------
RKSimon wrote:
> fhahn wrote:
> > RKSimon wrote:
> > > Why are you forcing the alignment ?
> > I think we have to update the alignment of the scalar load, because after applying an offset to the pointer it may not be aligned as specified on the original load. I think we should be able to use the common alignment between the alignment on the load and the scalar type. WDYT?
> Yes, using a common alignment would make more sense
I updated the code to use the common alignment.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:876
+        // to memory.
+        if (NumInstChecked == 6 || I.mayWriteToMemory())
+          return false;
----------------
RKSimon wrote:
> Pull out the magic number (MaxInstChecked = 6 ?)
I updated the code to use `MaxInstrsToScan` which was added by an earlier patch.


================
Comment at: llvm/test/Transforms/VectorCombine/AArch64/load-extractelement-scalarization.ll:253
 ; Scalarizing the load for multiple constant indices may not be profitable.
 define i32 @load_multiple_extracts_with_constant_idx(<4 x i32>* %x) {
 ; CHECK-LABEL: @load_multiple_extracts_with_constant_idx(
----------------
spatel wrote:
> Let me know if I'm overlooking it, but I don't see a positive test for this pattern (multiple extracts of a single load).
> Is there some vector type variant (float?) of this test that will transform for AArch64?
> If not, we might get x86 or some other target to trigger on this test as-is.
Yes I think this case was missing. I added `load_multiple_extracts_with_constant_idx_profitable` to cover that I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100273