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

[Jun Bum Lim via llvm-commits]

junbuml added a comment.

Thanks Chad for the review. I will address your comments soon.


================
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.
----------------
mcrosier wrote:
> Once this gets committed please file a PR unless you plan on doing the follow up work.
Yes, I will do that.

================
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;
----------------
mcrosier wrote:
> 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.
Agree. If the first two condition is true and the the third condition is false, that means OverwriteComplete. As you pointed, the third condition could be an assert().

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:454
@@ -433,1 +453,3 @@
+    return OverwriteBegin;
+
   return OverwriteUnknown;
----------------
mcrosier wrote:
> I believe you dropped a comment:
> 
>   // Otherwise, they don't completely overlap.
Oh.. Thanks ..

================
Comment at: test/Transforms/DeadStoreElimination/OverwriteStoreBegin.ll:16
@@ +15,3 @@
+
+define void @write0to4(i32* nocapture %p) {
+; CHECK-LABEL: @write0to4(
----------------
mcrosier wrote:
> 0to4 -> 0to3?
> 
> Should you decide to make this change, more instances below..
Thanks, I will fix them.

================
Comment at: test/Transforms/DeadStoreElimination/OverwriteStoreBegin.ll:52
@@ +51,3 @@
+
+define void @dontwrite0to4_align8(i32* nocapture %p) {
+; CHECK-LABEL: @dontwrite0to4_align8(
----------------
mcrosier wrote:
> 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).
Okay, I will comment it. 


http://reviews.llvm.org/D18906