This is an archive of the discontinued LLVM Phabricator instance.

Update MemorySSA in Local utils removing blocks.
ClosedPublic

Authored by asbirlea on Jun 29 2018, 2:35 PM.

Diff Detail

Event Timeline

asbirlea created this revision.Jun 29 2018, 2:35 PM
asbirlea updated this revision to Diff 160706.Aug 14 2018, 3:23 PM

Rebase and nit updates.

Looks fine to me, though I'd prefer if someone familiar with this code had a look, as well.

Looks like a nice improvement.

lib/Transforms/Utils/Local.cpp
2075

Yes, I think the deletion order doesn't matter.

asbirlea updated this revision to Diff 160857.Aug 15 2018, 11:05 AM

Address comment.

asbirlea marked an inline comment as done.Aug 15 2018, 11:05 AM
asbirlea added inline comments.
lib/Transforms/Utils/Local.cpp
2075

It seems to make sense to use this in the first iteration over the blocks in F that follows, but not the second one doing the erase, since that's better to be iterator based.
Also search DeadBlockSet instead of Reachable, assuming more blocks are live than dead.

asbirlea marked 2 inline comments as done.Aug 15 2018, 4:19 PM

Thanks for the improvement! Comments are inline.

lib/Transforms/Utils/Local.cpp
2075

I think maybe you can replace ToDeleteBBs with DeadBlockSet.
For assuming there are

more blocks are live than dead

, if you are interested, you can run CTMarks/have some other benchmarks to see whether there is performance degradation.

asbirlea updated this revision to Diff 161053.Aug 16 2018, 10:04 AM

Address comment.

NutshellySima accepted this revision.Aug 16 2018, 12:05 PM

Thanks for the change.
Please have a look at my new inline comments. (I have a quick glance at the source code of MemorySSA and I think it's fine; but it's better for you to check that).
LGTM.

lib/Transforms/Utils/Local.cpp
2101

We sometimes have BasicBlocks which are pending deletion in the DomTreeUpdater. (like that previously in DeferredDominance).

So, sometimes a same BasicBlock can be passed into MSSAU->removeBlocks multiple times in different calls to removeUnreachableBlocks. You need to be aware of that and check whether it is fine.

This revision is now accepted and ready to land.Aug 16 2018, 12:05 PM
asbirlea marked 2 inline comments as done.Aug 16 2018, 2:12 PM

Thank you for the review!

lib/Transforms/Utils/Local.cpp
2101

I believe that's fine. In MemorySSA, all memory accesses will be deleted for a dead block on the first call, subsequent calls will find an empty list.

This revision was automatically updated to reflect the committed changes.
asbirlea marked an inline comment as done.