[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:28
+public:
+  using CostType = int;
+
----------------
nhaehnle wrote:
> I'd argue that this should be `unsigned`. Are there places where instructions have negative costs?
I think in some places in the codebase there are comments saying that they explicitly chose signed integers for costs because they had complex arithmetic where the cost could go negative and I felt it wasn't quite right to restrict the type to being unsigned. Examples of where the signedness matters are transforms/optimisations where they have a budgeted cost and they decrement this budget by individual instruction costs to the point where the budget goes negative. At this point the optimisation bails out. When I wrote this class I thought that having signed integers for a cost type would help to avoid rewriting such algorithms to use unsigned budgets. Examples of such cases can be found in:

  lib/Transforms/Utils/ScalarEvolutionExpander.cpp
  lib/Transforms/Utils/SimplifyCFG.cpp

I guess it's also worth mentioning that this new InstructionCost is not intended to be fixed down in this patch. I imagine it's a process of evolution and over time as we use the InstructionCost class more to migrate more of the codebase we'll get a better feeling for what's needed and improve it as we go along.


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

https://reviews.llvm.org/D91174