[PATCH] [DAGCombine] Fix bug in MergeConsecutiveStores

[Sanjay Patel]

Hi Akira,

I don't feel qualified to fully review this patch; it's not clear to me that choosing the last store is the correct solution...
so I've just pointed out some nits. :)

I'm curious if you tracked down why this fails on aarch64 but not x86-64? Something in the x86 backend prevents the stores from merging?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10065
@@ -10064,3 +10064,3 @@
     // earliest store node which is *used* and replaced by the wide store.
-    if (StoreNodes[i].SequenceNum > StoreNodes[EarliestNodeUsed].SequenceNum)
-      EarliestNodeUsed = i;
+    if (StoreNodes[i].SequenceNum < StoreNodes[LatestNodeUsed].SequenceNum)
+      LatestNodeUsed = i;
----------------
Comment does not match code now: "earliest store" should be "latest store"?

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10135
@@ -10134,2 +10134,3 @@
 
   // Replace the first store with the new store
+  CombineTo(LatestOp, NewStore);
----------------
Comment does not match code now: "first store" should be "last store"?

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10523
@@ -10523,3 +10522,3 @@
     // earliest store node which is *used* and replaced by the wide store.
-    if (StoreNodes[i].SequenceNum > StoreNodes[EarliestNodeUsed].SequenceNum)
-      EarliestNodeUsed = i;
+    if (StoreNodes[i].SequenceNum < StoreNodes[LatestNodeUsed].SequenceNum)
+      LatestNodeUsed = i;
----------------
Comment does not match code now: "earliest store" should be "latest store"?

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10568
@@ -10566,2 +10567,3 @@
 
   // Replace the first store with the new store.
+  CombineTo(LatestOp, NewStore);
----------------
Comment does not match code now: "first store" should be "last store"?

================
Comment at: test/CodeGen/AArch64/merge-store.ll:6
@@ +5,3 @@
+
+; CHECK: stur {{d[0-9]+}}
+
----------------
Can the test case be minimized? Do we need @g1?
I think it would be good to also add check lines to makes sure that the loads were merged into the register as expected ahead of the store.

http://reviews.llvm.org/D8849

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/