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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

  (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).)



================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:28
+public:
+  using CostType = int;
+
----------------
I'd argue that this should be `unsigned`. Are there places where instructions have negative costs?


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:224-236
+inline InstructionCost operator*(const InstructionCost &LHS,
+                                 const InstructionCost &RHS) {
+  InstructionCost LHS2(LHS);
+  LHS2 *= RHS;
+  return LHS2;
+}
+
----------------
Dimensional analysis suggests that this particular overload of `operator*` should not exist. Instead, we should have `(unsigned, InstructionCost)` and `(InstructionCost, unsigned)` overloads. By dimensional analysis I mean that we should think of InstructionCost as having an implicit unit, such as ns or nJ or whatever. Multiplying two InstructionCosts would give us square seconds or square joules, which is nonsensical.

Similarly, the return type of this particular overload of `operator/` should just be unsigned.

Similar logic applies for the assignment operators.


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

https://reviews.llvm.org/D91174