[PATCH] D11166: WIP: Make MergeConsecutiveStores look at other stores on same chain

[hfinkel@anl.gov via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11400
@@ +11399,3 @@
+    // adjacent stores.
+    if (findBetterNeighborChains(ST)) {
+      // replaceStoreChain uses CombineTo, which handled all of the worklist
----------------
arsenm wrote:
> hfinkel wrote:
> > Could we be consistent and do this even when UseAA is false? This code ends up calling isAlias, but isAlias does things when UseAA is false (it just does more things when UseAA is true). If we did that, could be remove the special case for UseAA in getStoreMergeAndAliasCandidates (because we'd again have a single canonical form for the store chains).
> > 
> That just exposes more instances of the same fundamental problem, that different combines currently expect different chain layouts. Ideally we could probably do this, however that will require more work. Doing this without AA will move more chains, but most of the code is not ready for this (which is why I assume CombinerAA is not on by default now).
> 
> If you remove the UseAA check here (and in current code), you many of the same test failures as you do if you enable combiner-aa by default. Eventually when more combines are fixed to look at the same chain, we could probably do this.
> Doing this without AA will move more chains, but most of the code is not ready for this (which is why I assume CombinerAA is not on by default now).

It is on by default for some targets (whatever targets return true from useAA() in their TargetSubtargetInfo subclass).

> If you remove the UseAA check here (and in current code), you many of the same test failures as you do if you enable combiner-aa by default. Eventually when more combines are fixed to look at the same chain, we could probably do this.

Okay, please add a FIXME explaining this here.



http://reviews.llvm.org/D11166