[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
llvm-commits at lists.llvm.org
Sun Jun 4 12:46:38 PDT 2017
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
More information about the llvm-commits
mailing list