[PATCH] D30703: [DSE] Merge stores when the later store only writes to memory locations the early store also wrote to.

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Hi @sanjoy. Can you take a look at the revised changes?

Thank you,
Filipe



================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:446
+      Earlier.Size >= Later.Size &&
+      uint64_t(LaterOff - EarlierOff) + Later.Size <= Earlier.Size) {
+    DEBUG(dbgs() << "DSE: Partial overwrite an earlier load [" << EarlierOff
----------------
sanjoy wrote:
> Can `LaterOff - EarlierOff` sign overflow (in artificial but legal cases)?
Probably yes. Redoing the math.

1 - Make sure `LaterOff` is between `EarlierOff` and `EarlierOff + Earlier.Size`. We know that addition won't overflow (if it does, the bug is in `GetPointerBaseWithConstantOffset`), so we'll know we can subtract `EarlierOff` from `LaterOff` later without problems, since they're ordered that way and the difference will be smaller than `Earlier.Size`.



================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:452
+    // TODO: Maybe come up with a better name?
+    return OW_PartialEarlierWithFullLater;
+  }
----------------
sanjoy wrote:
> Shouldn't this be `FullEarlierWithPartialLater`?
> 
> In a second change, I'd suggest changing the naming scheme to:
> 
> `OW_Begin` - `OW_Prefix`
> `OW_Complete` - `OW_Complete`
> `OW_End` - `OW_Suffix`
> `OW_PartialEarlierWithFullLater` - `OW_Partial`
> `OW_Unknown` - `OW_Unknown`
> 
> 
Partial earlier store gets overwritten by a latter store (and the latter store doesn't write outside the "full" earlier store).

It's a weird way to phrase it, but I don't think changing it to your suggestion makes much sense (but I think I know what you mean).
I do think your renaming proposal for the whole enum makes sense, so I'll do that one.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1200
+            // Replace earlier, wider, store
+            DepWrite->replaceAllUsesWith(SI);
+            size_t Idx = InstrOrdering.lookup(DepWrite);
----------------
sanjoy wrote:
> Why is this needed?  `store` instructions should not have users (and a `WeakTrackingVH` on a `store` is most likely doing the wrong thing).
Removed.


https://reviews.llvm.org/D30703