[PATCH] D18906: [DeadStoreElimination] Shorten beginning of memset overwritten by later stores

[Chad Rosier via llvm-commits]

mcrosier requested changes to this revision.
mcrosier added a comment.
This revision now requires changes to proceed.

The patch looks to be in good shape generally.  I've inlined a few nits and questions/suggestions.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:278
@@ -277,4 +277,3 @@
 
-/// isShortenable - Returns true if this instruction can be safely shortened in
-/// length.
-static bool isShortenable(Instruction *I) {
+/// isShortenableAtTheEnd - Returns true if the end of this instruction can be
+/// safely shortened in length.
----------------
The preferred doxygen style is to drop the function name in the comments.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:300
@@ -299,1 +299,3 @@
 
+/// isShortenableAtTheBeginning - Returns true if the beginning of this
+/// instruction can be safely shortened in length.
----------------
The preferred doxygen style is to drop the function name in the comments.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:303
@@ +302,3 @@
+static bool isShortenableAtTheBeginning(Instruction *I) {
+  // FIXME: Handle only memset for now. Supporting memcpy should be easily done
+  // by offsetting the source address.
----------------
Once this gets committed please file a PR unless you plan on doing the follow up work.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:348
@@ -338,4 +347,3 @@
 /// isOverwrite - Return 'OverwriteComplete' if a store to the 'Later' location
-/// completely overwrites a store to the 'Earlier' location.
-/// 'OverwriteEnd' if the end of the 'Earlier' location is completely
-/// overwritten by 'Later', or 'OverwriteUnknown' if nothing can be determined
+/// completely overwrites a store to the 'Earlier' location. 'OverwriteEnd' if
+/// the end of the 'Earlier' location is completely overwritten by 'Later',
----------------
location. -> location,

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:429
@@ -418,3 +428,3 @@
 
   // The other interesting case is if the later store overwrites the end of
   // the earlier store
----------------
The other interesting case -> Another interesting case

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:442
@@ -431,2 +441,3 @@
 
-  // Otherwise, they don't completely overlap.
+  // We also need to check if the later store overwrites the beginning of
+  // the earlier store
----------------
We also need -> Finally, we also need

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:452
@@ +451,3 @@
+  if (LaterOff <= EarlierOff && int64_t(LaterOff + Later.Size) > EarlierOff &&
+      int64_t(LaterOff + Later.Size) < int64_t(EarlierOff + Earlier.Size))
+    return OverwriteBegin;
----------------
I suspect this condition 'int64_t(LaterOff + Later.Size) < int64_t(EarlierOff + Earlier.Size)' will always be true because the OverwriteComplete case will  return true when LaterEnd is >= EarlyEnd.

If I am correct and the condition is true, perhaps add an assert that it's always true.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:454
@@ -433,1 +453,3 @@
+    return OverwriteBegin;
+
   return OverwriteUnknown;
----------------
I believe you dropped a comment:

  // Otherwise, they don't completely overlap.

================
Comment at: test/Transforms/DeadStoreElimination/OverwriteStoreBegin.ll:3
@@ +2,3 @@
+
+define void @write4to8(i32* nocapture %p) {
+; CHECK-LABEL: @write4to8(
----------------
4to8 -> 4to7?

================
Comment at: test/Transforms/DeadStoreElimination/OverwriteStoreBegin.ll:16
@@ +15,3 @@
+
+define void @write0to4(i32* nocapture %p) {
+; CHECK-LABEL: @write0to4(
----------------
0to4 -> 0to3?

Should you decide to make this change, more instances below..

================
Comment at: test/Transforms/DeadStoreElimination/OverwriteStoreBegin.ll:52
@@ +51,3 @@
+
+define void @dontwrite0to4_align8(i32* nocapture %p) {
+; CHECK-LABEL: @dontwrite0to4_align8(
----------------
Perhaps add a comments stating why this transformation is unsafe (i.e., we must not change the alignment of the earlier write, if I'm not mistaken).


http://reviews.llvm.org/D18906