This is an archive of the discontinued LLVM Phabricator instance.

[MachineSink] Disable if there are any irreducible cycles
ClosedPublic

Authored by nikic on Mar 2 2022, 1:26 AM.

Details

Summary

This is an alternative to D120330, which disables MachineSink for functions with irreducible cycles entirely. This avoids both the correctness problem, and ensures we don't perform non-profitable sinks into cycles. At the same time, it may also disable profitable sinks in the same function.

Diff Detail

Event Timeline

nikic created this revision.Mar 2 2022, 1:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 1:26 AM
nikic requested review of this revision.Mar 2 2022, 1:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 1:26 AM
shchenz accepted this revision.Mar 2 2022, 7:22 AM

LGTM. Thanks for fixing this. I will check MachineCycleInfo for the main branch if you don't plan to do so.

This revision is now accepted and ready to land.Mar 2 2022, 7:22 AM
This revision was landed with ongoing or failed builds.Mar 2 2022, 7:57 AM
This revision was automatically updated to reflect the committed changes.
dmgreen added a subscriber: dmgreen.Mar 3 2022, 3:45 AM

Hello. This seems to be increasing codesize by quite a large amount in places at -Oz. Up to 9% in one case! (They tend to be machine-outliner related when they get that large). It seems to be increasing codesize in general, I would guess because having irreducible loops somewhere throughout a large function can be quite common.

nikic added a comment.Mar 3 2022, 4:10 AM

Hello. This seems to be increasing codesize by quite a large amount in places at -Oz. Up to 9% in one case! (They tend to be machine-outliner related when they get that large). It seems to be increasing codesize in general, I would guess because having irreducible loops somewhere throughout a large function can be quite common.

Yes, having some irreducible control flow in a larger function is a problematic case for this approach. The alternatives to this change are D120330 or a revert of D86864. I'm fine with any of these three three options myself.

My initial thought is to bail out only if the loop containing MBB or SuccToSinkTo in isProfitableToSinkTo contains irreducible cfg or not. I guess we checked the whole function instead of the single loop because LoopBlocksRPO(which is suitable to check a single loop) does not have a MachineLoop version?

I plan to do this based on MachineCycleInfo for the main branch, But I need more time. If this code size issue caused by this approach is some kind of seriousness and urgent, yes, I am also OK to use the alternatives, D120330, or revert D86864.