This is an archive of the discontinued LLVM Phabricator instance.

[NFC][SCEV] Piping to pass new SCEVCheapExpansionBudget option into SCEVExpander::isHighCostExpansionHelper()
ClosedPublic

Authored by lebedev.ri on Jan 30 2020, 6:37 AM.

Details

Summary

In future patches`SCEVExpander::isHighCostExpansionHelper()` will respect the budget allocated by performing TTI cost modelling.
This is a fully NFC patch to make things reviewable.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jan 30 2020, 6:37 AM

NFC, isHighCostExpansionHelper() should take BudgetRemaining by reference.

mkazantsev added inline comments.Feb 3 2020, 2:02 AM
llvm/include/llvm/Analysis/ScalarEvolutionExpander.h
186

Should it be unsigned?

331

Please make all budgets either signed or unsigned. This should be consistent.

lebedev.ri marked 3 inline comments as done.Feb 3 2020, 2:11 AM
lebedev.ri added inline comments.
llvm/include/llvm/Analysis/ScalarEvolutionExpander.h
186

Absolutely not.
For signed, it is obvious how to check that we've out of budget (it became negative),
while for unsigned i would need to be checking the remaining budget vs it's adjustment.

lebedev.ri marked an inline comment as done.Feb 3 2020, 2:11 AM
lebedev.ri marked an inline comment as done.Feb 3 2020, 2:32 AM
lebedev.ri added inline comments.
llvm/include/llvm/Analysis/ScalarEvolutionExpander.h
331

There are two functions here: isHighCostExpansionHelper()
and isHighCostExpansion().

The helper is internal and self-recursive, and must have *signed*
budget parameter since it makes logic much better.

The isHighCostExpansion() is public, and is normally called with
just the SCEVCheapExpansionBudget param. I'm not sure it makes
much sense in accepting that param as signed,
because what would negative budget mean?

I suppose i could rename scev-cheap-expansion-budget param
to scev-cheap-expansion-threshold so all budgets are signed,
if that is better?

mkazantsev added inline comments.Feb 4 2020, 6:50 PM
llvm/include/llvm/Analysis/ScalarEvolutionExpander.h
331

And how it's going to work if the user specifies bugdet greater than max signed int?

lebedev.ri added inline comments.Feb 5 2020, 12:34 AM
llvm/include/llvm/Analysis/ScalarEvolutionExpander.h
331
lebedev.ri updated this revision to Diff 242582.Feb 5 2020, 6:05 AM

Rebased, NFC.
Let me know if/how this should be hardened.

mkazantsev accepted this revision.Feb 10 2020, 3:44 AM

Ok, thanks for clarification. Let them shoot their legs then. :) LGTM

This revision is now accepted and ready to land.Feb 10 2020, 3:44 AM

Ok, thanks for clarification. Let them shoot their legs then. :) LGTM

Thank you for the review!
To be noted, i'm not vehemently opposed to hardening, but not before
there is a clear idea what we should be actually doing for the undefined cases.

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.