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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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:
> > 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?


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

https://reviews.llvm.org/D92238