[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:674-675
         return StoreSource::Load;
       default:
-        return StoreSource::Unknown;
+        return StoreSource::Splat;
       }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141778