[PATCH] D156349: [DAG] Support store merging of vector constant stores

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19406
           // Make sure correctly size type is the correct type.
           Val = DAG.getBitcast(MemVT, Val);
         }
----------------
bjope wrote:
> bjope wrote:
> > Just a heads up. Found some problems with this downstream.
> > 
> > When it crashes it tries to merge these stores:
> > 
> > ```
> > t218: v4i32 = BUILD_VECTOR Constant:i32<0>, Constant:i32<0>, Constant:i32<0>, Constant:i32<0>
> > t221: ch = store<(store (s64) into %ir.arrayinit.start, align 1, !tbaa <0x557ab8701858>), trunc to v4i16> t4, t218, t104, undef:i16
> > t219: v2i32 = BUILD_VECTOR Constant:i32<0>, Constant:i32<0>
> > t194: ch = store<(store (s64) into %fixed-stack.0 + 15, align 1)> t4, t219, t102, undef:i16
> > ```
> > So the first store is a v4i32->v4i16 trunc store.
> > 
> > When we arrive on this line (19406) we got `MemVT`=v2i32 but `Val `is t218 from above (so that is a v4i32).
> > I think the correct thing would be to first truncate to v4i16 and then bitcast to v2i32 (given that we have a trunc store). And maybe the same bug exist for the `IsConstantSrc` false path as well.
> > 
> > I'll see if I can create an upstream reproducer for this. I only see it downstream in certain configs depending on which vector types that are legal, so not sure how easy it will be.
> > 
> > 
> I find it quite hard to find an in-tree reproducer.
> 
> I think I need to fulfil ` UseVector = (LastLegalVectorType > LastLegalType) && AllowVectors` to trigger this path. And then for example `TLI.storeOfVectorConstantIsCheap` need to return true in order to set LastLegalVectorType  (limiting the amount of possible targets a bit). And then it depends on which vector types that are legal. And if I for example use RISCV as target it seems like it folds the truncate of a build vector, rather than introducing a truncating store.
> 
Finally some luck:
```
; RUN: llc -mtriple=x86_64-unknown-unknown -mcpu=skx -o - %s
define void @foo(ptr %p, ptr %q) {
  %v = insertelement <8 x i64> zeroinitializer, i64 0, i32 1
  %tranc = trunc <8 x i64> %v to <8 x i8>
  %p1 = getelementptr i8, ptr %p, i32 0
  %p2 = getelementptr i8, ptr %p, i32 8
  %p3 = getelementptr i8, ptr %p, i32 128
  store <8 x i8> %tranc, ptr %p1
  store <8 x i8> zeroinitializer, ptr %p2
  ret void
}
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156349