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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D92238#2521095 <https://reviews.llvm.org/D92238#2521095>, @lebedev.ri wrote:

> Won't we now not stop as soon as the budget is invalidated?

That should be no different than before, `isHighCostExpansionHelper` is not recursive. The function that calls it, `isHighCostExpansion`, will still stop immediately when the cost is invalidated.

> Perhaps if `InstructionCost::operator<` can't be fixed, it needs to be ripped out.
> I'm sorry, i don't have useful feedback other than "the new abstraction is bad".

The part of the new abstraction that I believe may require more thought is the signedness of InstructionCost. If Cost would be unsigned, the code is already expected to deal with wrapping values due to underflow and `InstructionCost::operator<` would work as expected. There would be no need to add additional checks for validity.  Another possibility would be to return Invalid when there is an underflow, because if there is a negative cost at any point this is probably a bug. That would still mean changing the compare "Budget < 0" in this file, but there are other ways to write it such as @ctetreau's suggests.

Is this something to consider changing at some point?

> I'm curious: what's the objection to having the budget be a negative number, and adding costs instead of subtracting?

Personally I find this a bit counter-intuitive, because a budget is always a positive value that is subtracted from.

> We could also accumulate costs in a separate variable and compare `Cost < Budget`, which will also do the right thing in the face of an invalid cost infecting the running total. In this case, `Budget` doesn't even need to be an `InstructionCost`.

Yes, I quite like that suggestion.


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

https://reviews.llvm.org/D92238