[PATCH] D100565: [TTI] NFC: Change getIntImmCost[Inst|Intrin] to return InstructionCost

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 2 inline comments as done.
sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp:390
     }
-    ConstIntCandVec[Itr->second].addUser(Inst, Idx, Cost);
+    ConstIntCandVec[Itr->second].addUser(Inst, Idx, *Cost.getValue());
     LLVM_DEBUG(if (isa<ConstantInt>(Inst->getOperand(Idx))) dbgs()
----------------
ctetreau wrote:
> c-rhodes wrote:
> > Does there need to be a check the cost is valid before calling `getValue`?
> Code that handles the invalid case would be a functional change. This follows the philosophy of "the code previously assumed all costs are valid, so we will assume it's valid now and handle the crashes in future functional changes patches if we see them"
@c-rhodes seems I wrote this comment but forgot to publish it. The assert in the dereference would fail if the cost is Invalid. We can assume the cost of an instruction in a basic-block is valid, because otherwise it would suggest the instruction cannot be code-generated. This would mean that either the cost-model is broken (it gives an invalid cost for something that _can_ be code-generated), or it means that the IR contains instructions that shouldn't have been created in the first place. As Chris says, changing the code to solve such an issue means making a functional change, and we can just work on the assumption that the cost was valid before this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100565