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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolutionExpander.h:331
     bool isHighCostExpansionHelper(const SCEV *S, Loop *L,
-                                   const Instruction *At,
+                                   const Instruction *At, int &BudgetRemaining,
                                    const TargetTransformInfo *TTI,
----------------
mkazantsev wrote:
> lebedev.ri wrote:
> > mkazantsev wrote:
> > > Please make all budgets either signed or unsigned. This should be consistent.
> > 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?
> And how it's going to work if the user specifies bugdet greater than max signed int? 
And if it's signed, what happens if the specified value overflows during it's multiplication by `TargetTransformInfo::TCC_Basic`?

All these options are by definition internal, non-public. I'm not very sure
how much time we want to spend ensuring that one can't use it as a footgun.
Besides, this merely models preexisting practice:
https://github.com/llvm/llvm-project/blob/a3d489e87e8243bdb0eff947a38006b039dff8c0/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L93-L105
https://github.com/llvm/llvm-project/blob/a3d489e87e8243bdb0eff947a38006b039dff8c0/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L2017
https://github.com/llvm/llvm-project/blob/a3d489e87e8243bdb0eff947a38006b039dff8c0/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L2334-L2335
https://github.com/llvm/llvm-project/blob/a3d489e87e8243bdb0eff947a38006b039dff8c0/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L3011-L3012


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73705/new/

https://reviews.llvm.org/D73705