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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
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;
+}
+
----------------
nhaehnle wrote:
> 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.
Fundamentally the InstructionCost is really just a wrapper for an integer with an additional state attached. We're multiplying two integers, i.e. just the same as builtin operator*(int,int). The only difference between a traditional integer operator* and the InstructionCost variant is that we are propagating a state - see the propagateState() function.

We also tried to avoid complexities of modelling it too much like an FP value by talking about 'infinity' (which leads to the question as to what 0 * Infinity means). Instead, this class is more simple and pragmatic, which turns out to be sufficient for all uses of this class. If one of the values in an expression is Invalid, the resulting expression is Invalid as well.


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

https://reviews.llvm.org/D91174