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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @lebedev.ri, when we introduced InstructionCost, we left open the possibility of changing the class based on new insights on how costs are used in TargetTransformInfo and IR passes.

In this patch you pointed out a valid concern. It is indeed unexpected to need to have to check `isValid()` when wanting to know if a budget has gone negative after subtracting a cost, because even though InstructionCost is signed, subtracting Invalid currently results in `Invalid`, not `-Invalid`.

I had a chat with @ctetreau offline about changing InstructionCost by making the cost `unsigned` (i.e. a cost can only ever be positive), or possibly restricting the class's operators that it can only ever be incrementing, so that sorting Invalid after a valid cost value would no longer be strange/unexpected. I think this would address your fundamental concerns with the abstraction. There is some investigation required on what exactly the right solution would be, but I'd be happy to follow this up separately.

For the use-case in this patch (SCEVExpander), I think we now have a suitable solution that makes sense and is future proof for whatever changes we may make to InstructionCost. If you have no strong objections to the current approach (separate Cost and Budget variables), are you happy to accept the patch so that we can move this forward?


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

https://reviews.llvm.org/D92238