[PATCH] D92238: [SCEVExpander] Migrate costAndCollectOperands to use InstructionCost.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

I have come to the realisation that an abstraction around InstructionCost specific for budgeting is actually quite useful.

@lebedev.ri are you happy with these changes?



================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2348-2349
     SmallVectorImpl<SCEVOperand> &Worklist) {
-  if (BudgetRemaining < 0)
+  if (!BudgetRemaining.isValid() || BudgetRemaining < 0)
     return true; // Already run out of budget, give up.
 
----------------
sdesmalen wrote:
> lebedev.ri wrote:
> > sdesmalen wrote:
> > > lebedev.ri wrote:
> > > > What happens without the `isValid()` check?
> > > Without the `isValid()` check it defaults to the total ordering for InstructionCost where all valid costs < Invalid.
> > > This means that `BudgetRemaining < 0` would evaluate to `false`, where instead we want to return `true` from this function to signal this is a high cost expansion.
> > Uh oh.
> > 
> > So if every single check now needs to not be forgotten to be prefixed with `isValid()`,
> > doesn't that imply that the default is wrong, and it should be `Invalid < all valid costs` instead?
> > I'm guessing not, because i guess `0 > BudgetRemaining` will then break.
> > 
> > So alternatively, why not at least make it easy to detect this issue,
> > and instead assert within the `InstructionCost` that costs must be valid to do such comparison?
> In D91174 @ctetreau argued in favour of having a total ordering over asserting that the costs must be valid (https://reviews.llvm.org/D91174#2422681).
> 
> Probably the most prominent use-case where an InstructionCost is used in comparisons is:
> 
>   if (CostX < CostY)
>     // replace with X
> 
> Here the total-ordering is desirable, i.e. if CostX is invalid, don't replace with X. Otherwise, replace if CostX is smaller than CostY or CostY is invalid. We want to avoid having to rewrite these cases to:
> 
>   if ((CostX.isValid() && !CostY.isValid()) ||
>        (CostX.isValid() && CostY.isValid() && CostX < Cost))
>     // replace with X
> 
>   // All cases would need to be covered if CostX < CostY would *assert*
>   // they most both be valid.
> 
> I think that in this particular case - working with cost as a budget - there is extra confusion because the code is comparing directly with a constant, which makes it easy to forget that this is not a 'normal' comparison, but is in fact a comparison between InstructionCosts which has total ordering semantics.
> 
> What I could do is:
> * Mark `InstructionCost::operator<(int)` operator as `delete`, so that the above statement no longer compiles. This means only comparisons between InstructionCosts are supported.
> * Add a new method `bool InstructionCost::isNegativeValue()` which returns `true` if the value is Valid and less than 0.
> * Update this patch to use `isNegativeValue()` instead.
> 
> Would that be a suitable solution?
Last week I've been working to update multiple passes and interfaces to work on InstructionCost, and I found that for most, if not all, other cases where InstructionCost is used the total ordering is sufficient and sufficiently clear.  Therefore I don't think my above suggestion makes sense, so please ignore it.


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

https://reviews.llvm.org/D92238