[PATCH] D91174: [Analysis] Introduce a new InstructionCost class
Christopher Tetreault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 10 10:09:33 PST 2020
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
More information about the llvm-commits
mailing list