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

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10787
@@ +10786,3 @@
+    // alias.
+    // TODO: This should be the common (only?) case if combiner-aa is
+    // enabled. Should we skip or remove the code looking at consecutive chains?
----------------
As I said before, this comment is misleading. That situation also commonly occurs when expanding memcpy intrinsics, expanding vector stores of illegal type, etc.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10804
@@ +10803,3 @@
+
+    return;
+  }
----------------
Based on your review comments, you're relying here on the fact that, when AA is being used, FindBetterChain canonicalizes non-aliasing stores to be on the same chain. This needs at least a comment.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11400
@@ +11399,3 @@
+    // adjacent stores.
+    if (findBetterNeighborChains(ST)) {
+      // replaceStoreChain uses CombineTo, which handled all of the worklist
----------------
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).


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14222
@@ +14221,3 @@
+      } else if (LoadSDNode *Ldn = dyn_cast<LoadSDNode>(NextInChain)) {
+        ChainedLoads.push_back(Ldn);
+        NextInChain = Ldn->getChain().getNode();
----------------
We don't seem to use this for anything other than this. Remove it?



http://reviews.llvm.org/D11166