This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Simplify CFG in SCCP as well
ClosedPublic

Authored by nikic on Jun 29 2022, 2:39 AM.

Details

Summary

Currently, we only remove dead blocks and non-feasible edges in IPSCCP, but not in SCCP. I'm not aware of any strong reason for that difference, so this patch updates SCCP to perform the CFG cleanup as well.

Compile-time impact seems to be pretty minimal: http://llvm-compile-time-tracker.com/compare.php?from=f65c88c42fdd0e46d16fe31737e6627c56de77c3&to=d9d221a4687368b26083ca34a230e69f9f0fb59b&stat=instructions

For the test case from https://reviews.llvm.org/D126962#3611579 the result after -sccp now looks like this: https://gist.github.com/nikic/d50c03ec140bf9de217c9960d4f60a08 -jump-threading does nothing on this, but -simplifycfg will produce the optimal result.

Diff Detail

Event Timeline

nikic created this revision.Jun 29 2022, 2:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 2:39 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nikic requested review of this revision.Jun 29 2022, 2:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 2:39 AM
fhahn accepted this revision.Jun 29 2022, 6:44 AM

LGTM, cleaning up unreachable block here early seems reasonable to me as it doesn't seems to be a big compile-time hit, thanks!

This revision is now accepted and ready to land.Jun 29 2022, 6:44 AM
This revision was landed with ongoing or failed builds.Jun 30 2022, 12:25 AM
This revision was automatically updated to reflect the committed changes.
cfang added a subscriber: cfang.Jul 12 2022, 10:08 AM

This patch triggered a correctness issue in running mixbench-ocl-alt.
I am not familiar with the CFG in SCCP pass at all. But the comments
in the code seems suggest we should not change the CFG:

If we decided that there are basic blocks that are dead in this function,
delete their contents now. Note that we cannot actually delete the blocks,
// as we cannot modify the CFG of the function.

This patch triggered a correctness issue in running mixbench-ocl-alt.
I am not familiar with the CFG in SCCP pass at all. But the comments
in the code seems suggest we should not change the CFG:

If we decided that there are basic blocks that are dead in this function,
delete their contents now. Note that we cannot actually delete the blocks,
// as we cannot modify the CFG of the function.

The comment should be irrelevant, the pass is not longer claiming to preserve the CFG, so that should be fine. If there is an incorrect transformation happening, we would need a reproducer.