[PATCH] D143069: [DAGCombine] Allow DAGCombine to remove dead masked stores

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11369
+        !MST->getBasePtr().isUndef() &&
+        ((Mask == MST1->getMask() && MST->getMemoryVT().getStoreSize() ==
+                                         MST1->getMemoryVT().getStoreSize()) ||
----------------
dtemirbulatov wrote:
> dtemirbulatov wrote:
> > sdesmalen wrote:
> > > > fixed error with the same mask but different type size by forbid to delete the store
> > > I'm not sure if this is the reason you made this change, but the test that previously removed the redundant store:
> > > ```define void @dead_masked_store_same_mask_bigger_type(<vscale x 4 x i16> %val, <vscale x 4 x i32> %val1, ptr %a, <vscale x 4 x i1> %mask) {
> > > ; CHECK-LABEL: dead_masked_store_same_mask_bigger_type:
> > > ; CHECK:       // %bb.0:
> > > ; CHECK-NEXT:    st1w { z1.s }, p0, [x0]
> > > ; CHECK-NEXT:    ret
> > >   call void @llvm.masked.store.nxv4i16(<vscale x 4 x i16> %val, ptr %a, i32 4, <vscale x 4 x i1> %mask)
> > >   call void @llvm.masked.store.nxv4i32(<vscale x 4 x i32> %val1, ptr %a, i32 4, <vscale x 4 x i1> %mask)
> > >   ret void
> > > }
> > > ```
> > > Is now doing this:
> > > ```define void @dead_masked_store_same_mask_bigger_type(<vscale x 4 x i16> %val, <vscale x 4 x i32> %val1, ptr %a, <vscale x 4 x i1> %mask) {
> > > ; CHECK-LABEL: dead_masked_store_same_mask_bigger_type:
> > > ; CHECK:       // %bb.0:
> > > ; CHECK-NEXT:    st1h { z0.s }, p0, [x0]
> > > ; CHECK-NEXT:    st1w { z1.s }, p0, [x0]
> > > ; CHECK-NEXT:    ret
> > >   call void @llvm.masked.store.nxv4i16(<vscale x 4 x i16> %val, ptr %a, i32 4, <vscale x 4 x i1> %mask)
> > >   call void @llvm.masked.store.nxv4i32(<vscale x 4 x i32> %val1, ptr %a, i32 4, <vscale x 4 x i1> %mask)
> > >   ret void
> > > }```
> > > 
> > > The second store is overwriting **all** the i16 elements that were stored in the first llvm.masked.store, with i32 elements from the second llvm.masked.store, so that means the first store can be removed right?
> > The mask is the same for both stores, but because we types are different starting then offsets of starting and ending affected elements different too. I think we have to keep both store in those cases.
> The mask is the same for both stores, but because types are different starting then offsets of starting and ending affected elements different too. I think we have to keep both store in those cases.
You're right, your current code is correct. The elements wouldn't necessarily line-up in memory, e.g.

  call void @llvm.masked.store.v4i16(<4 x i16> <a, b, c, d>, ptr %a, i32 4, <4 x i1> <1, 0, 0, 1>)  ; would store `a` and `d` resulting in   { a__d____ }
  call void @llvm.masked.store.v4i32(<4 x i32> <e, f, g, h>, ptr %a, i32 4, <4 x i1> <1, 0, 0, 1>)   ; would store `e` and `h` resulting in  { ee_d__hh }

so the first call can indeed not be removed.


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

https://reviews.llvm.org/D143069