This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Two entry phi select costs
AbandonedPublic

Authored by samparker on Aug 21 2020, 6:09 AM.

Details

Summary

In same manner as D82438, but in FoldTwoEntryPHINode, estimate the cost needed to insert the required selects and count this against the given budget. This will prevent some high cost conversions which IfConversion can later evaluate with better accuracy.

Diff Detail

Event Timeline

samparker created this revision.Aug 21 2020, 6:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2020, 6:09 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
samparker requested review of this revision.Aug 21 2020, 6:09 AM
lebedev.ri added inline comments.Aug 21 2020, 6:39 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
98–102

You need to bump it by that new cmp+sel cost, so +2.

2007

Put this into D86346?

2018

I'm not sure this does what you think it does..

samparker added inline comments.Aug 21 2020, 6:48 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
98–102

Where is the cmp coming from?

2018

Do you think this is wrong? What do you think it should be doing?

lebedev.ri added inline comments.Aug 21 2020, 6:50 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
98–102

Looking at getCmpSelInstrCost(), right, only the select, so +1.

lebedev.ri added inline comments.Aug 21 2020, 6:52 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2018

Well, the function takes BudgetRemaining by value,
so this doesn't actually change the BudgetRemaining in the caller function.

samparker added inline comments.Aug 21 2020, 7:14 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2018

Right, this patch will need to be changed to work with D86346, but as a standalone patch it does what it should.

samparker updated this revision to Diff 290233.Sep 7 2020, 3:27 AM

Rebased after moving the select cost and validation into a standalone function. I've kept the threshold at 4 as a default, because this causes far fewer codegen changes (X86, AArch64, Arm) than trying to adjust it to accommodate the select by bumping it up to 5.

So we have two thresholds at play here?
That doesn't seem right to me..

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2026–2028

here

2044–2049

and here

lebedev.ri requested changes to this revision.Sep 10 2020, 12:43 AM

Hold on, how does this interact with D86346?

This revision now requires changes to proceed.Sep 10 2020, 12:43 AM
lebedev.ri resigned from this revision.Jan 12 2023, 4:46 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

This revision now requires review to proceed.Jan 12 2023, 4:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 4:46 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
samparker abandoned this revision.Jan 12 2023, 9:04 PM