This is an archive of the discontinued LLVM Phabricator instance.

LibcallsShrinkWrap doesn't preserve the CFG
ClosedPublic

Authored by davide on Nov 7 2016, 5:33 PM.

Details

Summary

This is the dominator before/after

Computed:

Inorder Dominator Tree:

[1] %0 {0,1}

Actual:

Inorder Dominator Tree:

[1] %0 {0,5}
  [2] %cdce.call {1,2}
  [2] %cdce.end {3,4}

Diff Detail

Event Timeline

davide updated this revision to Diff 77125.Nov 7 2016, 5:33 PM
davide updated this revision to Diff 77126.
davide retitled this revision from to LibcallsShrinkWrap doesn't preserve the CFG.
davide updated this object.
davide added reviewers: xur, eli.friedman.
davide added a subscriber: llvm-commits.

Updated testcase.

xur accepted this revision.Nov 7 2016, 5:40 PM
xur edited edge metadata.

Thanks for the fix. Look good to me.
It may be better to rename the test case using bug_id.

This revision is now accepted and ready to land.Nov 7 2016, 5:40 PM

Can you use -verify-dom-info to write a testcase, as opposed to a run line with three passes? (If that doesn't actually work, please ignore this.)

LibcallsShrinkWrap should preserve GlobalsAAWrapperPass, at least. domtree would probably be easy to preserve, but that can be done as a followup.

Please make sure to upload patches with full context.

davide updated this revision to Diff 77135.Nov 7 2016, 7:22 PM
davide edited edge metadata.

Update after Eli's comments.

davide added a comment.Nov 7 2016, 7:23 PM

Can you use -verify-dom-info to write a testcase, as opposed to a run line with three passes? (If that doesn't actually work, please ignore this.)

I tried before, but that doesn't actually work.

LibcallsShrinkWrap should preserve GlobalsAAWrapperPass, at least. domtree would probably be easy to preserve, but that can be done as a followup.

Done.

Please make sure to upload patches with full context.

Sorry, for some reason I forgot to add -U to my git diff invocation..

davide updated this revision to Diff 77137.Nov 7 2016, 7:52 PM

Renamed test file.

Looked into the test a little; I guess to trigger -verify-dom-info, you need to compute domtree before -libcalls-shrinkwrap, and have a pass which requires domtree after it. So "opt -domtree -libcalls-shrinkwrap -instcombine -verify-dom-info" triggers verification, for example. I think that's a little bit better than dragging in gvn.

davide updated this revision to Diff 77214.Nov 8 2016, 10:44 AM

Updated the pipeline. Also added a comment to explain what's going on there.

davide updated this revision to Diff 77215.Nov 8 2016, 10:46 AM
efriedma accepted this revision.Nov 8 2016, 10:52 AM
efriedma added a reviewer: efriedma.

LGTM.

I feel like we should probably make a new directory for libcalls-shrinkwrap tests at some point, but you don't have to do that here.

This revision was automatically updated to reflect the committed changes.