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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

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

> Then please fix the `InstrouctionCost` comparison operators to do the right thing to avoid most of the changes in this diff in the first place.

`InstructionCost` does do the right thing. `InstructionCost` is isomorphic to `Optional<int>`, except in 99% of cases, you want an invalid cost to be considered greater than any other cost. Does `int` do the wrong thing because `std::numeric_limits<int>::max() + 1 < std::numeric_limits<int>::max()`?

I don't think that `Cost < 0 || Cost.isInvalid()` is an unreasonable change to request. Especially considering all the other code that uses InstructionCost that is written much more naturally due to the total ordering. An `isExceeded()` free function can be added to this file to ensure it's done correctly.



================
Comment at: llvm/include/llvm/Support/InstructionCost.h:238
+// of the complexities around Instruction's state.
+class InstructionBudget {
+  InstructionCost Budget;
----------------
ctetreau wrote:
> I think this new class is uneccesary. Can't we just write `0 > Cost` for any case where we would use  `Cost.isExceeded()`?
I guess this doesn't work either. You have to do the isInvalid() call


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

https://reviews.llvm.org/D92238