[PATCH] D141778: [DAGCombiner][X86] `mergeConsecutiveStores()`: support merging splat-stores of the same value

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19413-19414
   const StoreSource StoreSrc = getStoreSource(StoredVal);
   if (StoreSrc == StoreSource::Unknown)
     return false;
 
----------------
This is dead now.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:674-675
         return StoreSource::Load;
       default:
-        return StoreSource::Unknown;
+        return StoreSource::Splat;
       }
----------------
lebedev.ri wrote:
> pengfei wrote:
> > lebedev.ri wrote:
> > > pengfei wrote:
> > > > Is this too strong to assume all the other cases are `Splat`? If this is the fact, why adding it in `StoreSource` rather than replacing `Unknown`?
> > > What do you mean by "too strong"?
> > > Originally we'd just return `StoreSource::Unknown` and bailout
> > > without looking at other stores. If the other stores *don't* store
> > > the same value, then we'll bailout just the same.
> > > 
> > > There is probably some generalization -- just because we've matched
> > > a more fine-grained `StoreSource`, doesn't mean that the stores
> > > *aren't* splatting the same value -- but that is not a required part
> > > of this enhancement.
> > > 
> > > As for `Unknown`, i don't know, but it seems not useful to just drop it,
> > > since it can be used to signal invalid state.
> > Alright, how about to use `MayBeSplat`?
> > 
> > I got the logic now. There is a `getStoreSource` before calling `getStoreMergeCandidates` and there is another `getStoreSource` in `getStoreMergeCandidates`. I think this is just a trick to make it works. It introduces uncertainty into the result. People may get unexpected result if they use this function somewhere else.
> Essentially, we want to guess what strategy will succeed
> to merge the stores, looking at a single store.
> `StoreSource` don't //really// describe the source,
> but names a strategy, it's not an ideal enum name.
> 
> But likewise, i'm not sure why `MayBeSplat` is a better strategy name?
> It either is if the strategy matched, or isn't if it didn't.
> 
> > I got the logic now. There is a getStoreSource before calling getStoreMergeCandidates and there is another getStoreSource in getStoreMergeCandidates.
> Yes.
> 
> > I think this is just a trick to make it works.
> What is?
> 
> > It introduces uncertainty into the result. People may get unexpected result if they use this function somewhere else.
> Sorry, i do not follow.
> `StoreSource` don't really describe the source, but names a strategy, it's not an ideal enum name.

I understand it now. Thanks for the explanation!
I had wrong assumption about the function. Please ignore the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141778