[PATCH] D91174: [Analysis] Introduce a new InstructionCost class

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D91174#2417172 <https://reviews.llvm.org/D91174#2417172>, @nhaehnle wrote:

>   (comparison being a total ordering)
>   /// This avoids having to add asserts the comparison operators that the states
>   /// are valid and users can test for validity of the cost explicitly.
>
> Counterpoint: The asserts cost nothing in a release build, and for software maintainability, having the asserts once in the comparison operator is better than having them at every callsite. Users of this class will forget to put them there.
>
> Worse, users may end up relying on the total ordering behavior, which seems like a bad idea to me: what does comparing 5 to Invalid mean? It might be tempting for users to use Invalid as a stand-in for Infinite (as suggested on llvm-dev), but Invalid does not behave arithmetically like one would except Infinite to behave. (For example, I would expect int(0) * InstructionCost(Infinite) == InstructionCost(0).)

For TypeSize we came across a similar issue, where we eventually decided to remove the overloaded comparison operators altogether.

Having an assert in `TypeSize::operator<` that verifies the scalable flag of two TypeSize objects matches might cause a crash in an assert-build. And because this is used for total orderings in sets/maps, we were concerned this would lead to spurious failures, especially because most LLVM code is still written with fixed width vectors in mind. Instead, we settled on using named methods if the operators were not mathematically correct, e.g. isKnownLT, isKnownGT. This function does what it says on the box, returns whether it knows that one object is less than (or greater then) the other. If it cannot determine it, it returns false.

For InstructionCost, we can take a similar approach:

- Remove all overloaded comparison operators [1]
- Add two new interfaces: `CostType getValueOr(CostType Other)` and `CostType getValueOrMax()` so that it can be left to the users of this code to interpret the 'Invalid' state and substitute it with a desired value for the comparison, e.g. `X.getValueOrMax() < Y.getValueOrMax()`

[1] we could possibly opt to keep __only__ `operator<` for total ordering used in maps/sets, and document its behaviour as using getValueOrMax.


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

https://reviews.llvm.org/D91174