[PATCH] [DAGCombine] Fix bug in MergeConsecutiveStores
Sanjay Patel
spatel at rotateright.com
Tue Apr 7 11:14:49 PDT 2015
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/
More information about the llvm-commits
mailing list