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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:674-675
         return StoreSource::Load;
       default:
-        return StoreSource::Unknown;
+        return StoreSource::Splat;
       }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141778