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

[Sanjoy Das via Phabricator via llvm-commits]

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

Some comments inline.



================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:307
 /// overwrites a store to the 'Earlier' location, 'OW_End' if the end of the
 /// 'Earlier' location is completely overwritten by 'Later', 'OW_Begin' if the
 /// beginning of the 'Earlier' location is overwritten by 'Later', or
----------------
Update doc.


================
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
----------------
Can `LaterOff - EarlierOff` sign overflow (in artificial but legal cases)?


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:452
+    // TODO: Maybe come up with a better name?
+    return OW_PartialEarlierWithFullLater;
+  }
----------------
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`




================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1178
+            unsigned BitOffsetDiff = (InstWriteOffset - DepWriteOffset) * 8;
+            unsigned ShiftAmount =
+                DL.isBigEndian()
----------------
I'd call this `LShiftAmount`.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1196
+                Earlier->getOrdering(), Earlier->getSynchScope(), DepWrite);
+            SI->copyMetadata(*DepWrite);
+            ++NumModifiedStores;
----------------
I'm not sure if `copyMetadata` is correct here.  Someone could be using metadata to track "the value I'm storing here is positive" which may no longer be true after the rewrite.  I think you should copy whitelisted metadata types (like `!tbaa`).


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


https://reviews.llvm.org/D30703