This is an archive of the discontinued LLVM Phabricator instance.

Recommit - [DSE]Enhance shorthening MemIntrinsic based on OverlapIntervals
ClosedPublic

Authored by junbuml on Jun 30 2016, 2:09 PM.

Details

Summary

Recommiting r275571 after fixing crash reported in PR28270.
Now we erase elements of IOL in deleteDeadInstruction().

Original Summary:
This change use the overlap interval map built from partial overwrite tracking to perform shortening MemIntrinsics.
Add test cases which was missing opportunities before.

Diff Detail

Repository
rL LLVM

Event Timeline

junbuml updated this revision to Diff 62413.Jun 30 2016, 2:09 PM
junbuml retitled this revision from to [DSE]Enhance shorthening MemIntrinsic based on OverlapIntervals.
junbuml updated this object.
junbuml added reviewers: hfinkel, mcrosier, eeckstein.
junbuml added a subscriber: llvm-commits.
junbuml updated this revision to Diff 62417.Jun 30 2016, 2:13 PM
junbuml updated this revision to Diff 62420.Jun 30 2016, 2:18 PM

Minor changes in the test cases.

mcrosier accepted this revision.Jul 14 2016, 11:46 AM
mcrosier edited edge metadata.

LGTM, with a few minor comments.

lib/Transforms/Scalar/DeadStoreElimination.cpp
823 ↗(On Diff #62420)

Would this look any better if the negation were pushed through? E.g.,

if (!(llvm::isPowerOf2_64(LaterOffset) && EarlierWriteAlign <= LaterOffset) &&
    !((EarlierWriteAlign != 0) && LaterOffset % EarlierWriteAlign == 0))
  return false;

I think that's the same logic.. :)

834 ↗(On Diff #62420)

Is this clang-formatted? Spacing looks odd.

914 ↗(On Diff #62420)
if (IntervalMap.empty())
  continue;
1052 ↗(On Diff #62420)

Is this how clang-format formatted the string?

test/Transforms/DeadStoreElimination/OverwriteStoreEnd.ll
106 ↗(On Diff #62420)

Please add a space between %base64_2 and =.

107 ↗(On Diff #62420)

Please add a space between %base64_3 and =.

This revision is now accepted and ready to land.Jul 14 2016, 11:46 AM
junbuml updated this revision to Diff 64026.Jul 14 2016, 12:20 PM
junbuml edited edge metadata.
junbuml marked 6 inline comments as done.

Addressed Chad's comment. Thanks Chad for the review.

lib/Transforms/Scalar/DeadStoreElimination.cpp
834 ↗(On Diff #62420)

Yes, I got this from clang-format.

1052 ↗(On Diff #62420)

Yes, this is what clang-format suggested.

Feel free to commit at your convenience, Jun.

This revision was automatically updated to reflect the committed changes.
junbuml updated this revision to Diff 64338.Jul 18 2016, 9:34 AM
junbuml retitled this revision from [DSE]Enhance shorthening MemIntrinsic based on OverlapIntervals to Recommit - [DSE]Enhance shorthening MemIntrinsic based on OverlapIntervals.
junbuml updated this object.
junbuml removed rL LLVM as the repository for this revision.

This was reverted in r275801 due to the crash reported in PR28270.

Now, I erase elements of IOL in deleteDeadInstruction() to catch all instructions removed.

LGTM. Thanks, Jun.

Recommitted in r276452.