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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

Overall, this seems reasonable to me.



================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:1
+//===- InstructionCost.h ----------------------------------------*- C++ -*-===//
+//
----------------
I think it's a bit strange that the new header is named `InstructionCost.h`, but all but 1 line of it is dedicated to defining a class called `GenericCost`. I feel like this header should be renamed `GenericCost.h`, and be put somewhere more central. (perhaps under llvm/Support). The `using InstructionCost = GenericCost<int>` should probably be moved out of this file. Perhaps TargetTransformInfo.h or CostModel.h?


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:35
+  typedef enum {
+    Normal = 0,
+    Invalid
----------------
BIKESHED: If you ask a person on the street what the opposite of "invalid" is, they probably won't answer "normal"


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:59
+
+  void setInvalid() { State = Invalid; }
+
----------------
should we have a `setValid` function? I'm thinking cases like `x / 0` being invalid, `select` (I would imagine) having `max(lhs, rhs)` cost, but `select(true, lhs, x / 0)` being valid, having the cost of the lhs.

(this is just an example. I'm sure `select(true, ..., ...)` doesn't naively do a `max`)


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:159
+
+  bool operator==(const GenericCost &RHS) const {
+    return Value == RHS.Value;
----------------
The comparators should do something with `isValid()`. I feel like this is going to be a source of bugs. I see three things you could do:

* assert: I'm not in love with this option. I think it's reasonable to ask `{7, Invalid} == {7, Valid}`
* define a total ordering: This is my preference. I think `forall x, y. {x, Invalid} > {y, Valid}` is good, because conceptually, an invalid cost has infinite cost.
* document a partial ordering: "nobody likes NaN semantics"


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:207-213
+  static GenericCost min(GenericCost LHS, GenericCost RHS) {
+    return LHS.Value < RHS.Value ? LHS : RHS;
+  }
+
+  static GenericCost max(GenericCost LHS, GenericCost RHS) {
+    return LHS.Value > RHS.Value ? LHS : RHS;
+  }
----------------
These should check isValid() (not necessary if you define a total ordering)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91174