[PATCH] D92238: [SCEVExpander] Migrate costAndCollectOperands to use InstructionCost.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D92238#2480072 <https://reviews.llvm.org/D92238#2480072>, @ctetreau wrote:

> In D92238#2452418 <https://reviews.llvm.org/D92238#2452418>, @lebedev.ri wrote:
>
>> I think this is conflating use-cases.
>> I'm not sure the entire `InstructionCost` should be used as a variable to track full/remaining cost over a several instructions.
>> Perhaps there should be a[nother] abstraction for that?
>
> Really, we were already doing this; we just didn't have a special cost type. We could leave the cost as an integer, but then the code would just become way more verbose (unwrapping every InstructionCost after computing them). Using the metaphor of money, a cost is a quantity of money. I think it's reasonable to subtract money from a budget. I suppose we could accumulate a running cost, and compare it with the budget that remains an integer, but that would just introduce another variable for (IMO) no good reason.

Like @ctetreau I don't see why the algorithm shouldn't be using InstructionCost. The class has enough capabilities that any invalid state now propagates quite naturally without too big changes (InstructionCost behaves mostly like a scalar int that carries extra state).
I've simplified the use of `*BudgetRemaining.getValue() < 0` to `BudgetRemaining < 0`, which hopefully makes the patch more palatable?



================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:247
     }
-    assert(BudgetRemaining >= 0 && "Should have returned from inner loop.");
+    assert(BudgetRemaining.isValid() && "Expected a valid cost");
+    assert(*BudgetRemaining.getValue() >= 0 &&
----------------
CarolineConcatto wrote:
> Maybe put this assertion before the loop? Is there any reason to check the Cost if BudgetRemaining.isValid()  is not valid?
> Or maybe you can remove this assert from here, because if we check isHighCostExpansionHelper this test is done in line 2346.
> Or maybe you can remove this assert from here, because if we check isHighCostExpansionHelper this test is done in line 2346.
That's true, but the assert here is only to verify the code/values are sane.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:2208
   const T *S = cast<T>(WorkItem.S);
   int Cost = 0;
   // Object to help map SCEV operands to expanded IR instructions.
----------------
CarolineConcatto wrote:
> Why you did not changed Cost to be InstructionCost?
Good catch!


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

https://reviews.llvm.org/D92238