[PATCH] D34921: [ConstantHoisting] Remove dupliate logic in constant hoisting

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Which testcases involve hoisting from allocas?  I don't think we really care what it does as long as it doesn't crash; instcombine canonicalizes allocas with a constant to use the constant "1" anyway.



================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:412
+    // only and intrinsics which can not be freely called with constants. So
+    // it's safe for us to collect constant candidates from all IntrinsicInsts.
+    if (canReplaceOperandWithVariable(Inst, Idx)
----------------
aoli wrote:
> efriedma wrote:
> > Are you sure this is true...?  For example, llvm.memcpy has an "len" parameter which can be hoisted, and an "align" parameter which can't.
> > 
> > Or maybe I'm misinterpreting what this comment is talking about?
> Yes, I checked all implementations of  method ` int getIntImmCost(Intrinsic::ID IID, unsigned Idx, const APInt &Imm, Type *Ty);`. There isn't any intrinsic which takes constant parameters only and can not be called freely with constant parameters.
> 
> The parameters in `llvm.memcpy` will never be hoisted because the cost of getting intrinsic immediate is always free for all targets. This is guaranteed in line 310
> 
> ```
> void ConstantHoistingPass::collectConstantCandidates(
>     ConstCandMapType &ConstCandMap, Instruction *Inst, unsigned Idx,
>     ConstantInt *ConstInt) {
>   unsigned Cost;
>   // Ask the target about the cost of materializing the constant for the given
>   // instruction and operand index.
>   if (auto IntrInst = dyn_cast<IntrinsicInst>(Inst))
>     Cost = TTI->getIntImmCost(IntrInst->getIntrinsicID(), Idx,
>                               ConstInt->getValue(), ConstInt->getType());
>   else
>     Cost = TTI->getIntImmCost(Inst->getOpcode(), Idx, ConstInt->getValue(),
>                               ConstInt->getType());
> 
>   // Ignore cheap integer constants.
>   if (Cost > TargetTransformInfo::TCC_Basic) {
> ```
> 
Oh, that's what you mean by "which can not be freely called with constants".  Maybe you could make the comment a bit more explicit (referencing getIntImmCost)?


https://reviews.llvm.org/D34921