This is an archive of the discontinued LLVM Phabricator instance.

Update MemorySSA BasicBlockUtils.
ClosedPublic

Authored by asbirlea on Apr 4 2018, 4:52 PM.

Diff Detail

Event Timeline

asbirlea created this revision.Apr 4 2018, 4:52 PM
asbirlea updated this revision to Diff 145099.May 3 2018, 2:53 PM

Rebase on dependent patch.

nhaehnle removed a subscriber: nhaehnle.May 4 2018, 2:00 AM
asbirlea updated this revision to Diff 160460.Aug 13 2018, 3:02 PM

Rebase, ready for review.

asbirlea added a reviewer: george.burgess.iv.

The general idea appears to be good, do you have any testcases reflect the change?

Also correct me if I'm wrong, it seems changes to MergeBlockIntoPredecessor can be separated from other changes.

Thank you for the comments!

I don't have testcases just for this change.
There are a couple of dependent patches in Phabricator (e.g. D45301, D47022 need to be rebased) that will use these changes.
The use of MemorySSA is disabled by default, but the dependent patches will update tests with a target that enables the use of MemorySSA (e.g. see tests in D40375).

Yes, I think the changes to MergeBlockIntoPredecessor could be split, but the overall purpose of adding an additional analysis to update is the same and the scope of the changes is fairly small.
If you think it will help review or clarity, I can split this up.

chandlerc accepted this revision.Aug 20 2018, 2:07 PM

I think just one patch is fine for this kind of API update.

If you want to test any of these changes, you can do so by writing a unittest, but we don't have an amazing track record here. See the *single* test we have in unittests/Transforms/Utils/BasicBlockUtils.cpp. While adding coverage there is good, if it proves hard to do or would require a bunch of work, I'm happy with relying on the usage in the subsequent patches.

The code LGTM. If you can't easily add a unittest, feel free to submit as-is. If you can add a test, I'm happy to look at that too.

This revision is now accepted and ready to land.Aug 20 2018, 2:07 PM
sanjoy accepted this revision.Aug 20 2018, 2:24 PM

lgtm

Thank you for the review!
I'll submit this one as is, and the usages will follow shortly.

This revision was automatically updated to reflect the committed changes.