[ConstantHoisting] Do not attempt to hoist ImmArgs

[lenary]

Summary

llvm::canReplaceOperandWithVariable used to have logic so that no arguments to
intrinsic functions could be replaced with variables. This lead to
ConstantHoisting adding an exception for intrinsic functions when checking if it
could hoist a particular argument.

Now that we have ImmArg annotations on intrinsic parameters, we query
them directly, and remove the exception in ConstantHoisting. This means
that ConstantHoisting will no longer ask for the cost of an intrinsic
immediate argument, and nor will it attempt to hoist it.

The RISC-V target hit this issue in development (alluded to in
rL364046), where we wanted it to default to medium-to-high immediate
costs for most immediates (on all calls and intrinsics), but this caused
IR verifier errors because ConstantHoisting was hoisting an ImmArg.

For context, in RISC-V, unless the instruction takes a 12-bit integer
immediate, materialising an integer constant takes 1 instruction for a
12-bit immediate, 2 instructions for a 32-bit immediate, and up to 5
instructions for a 64-bit immediate.

This change does not cause differences in the tests, because all targets
have engineered their immediate cost models to give a low cost to most
intrinsic arguments by default, as we have had to do in the RISC-V
backend. This change will allow backends to have a more accurate default
cost for materialisng integer constants, without triggering errors in
the IR verifier.

[arsenm]

See D58233

[lenary]

In D67396#1664905, @arsenm wrote:

See D58233

Oh neat, that patch looks much more comprehensive.

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

if (Inst->isCast())
     return;
 
   // Scan all operands.
   for (unsigned Idx = 0, E = Inst->getNumOperands(); Idx != E; ++Idx) {
     // The cost of materializing the constants (defined in
     // `TargetTransformInfo::getIntImmCost`) for instructions which only take
     // constant variables is lower than `TargetTransformInfo::TCC_Basic`. So
     // it's safe for us to collect constant candidates from all IntrinsicInsts.
-    if (canReplaceOperandWithVariable(Inst, Idx) || isa<IntrinsicInst>(Inst)) {
+    if (canReplaceOperandWithVariable(Inst, Idx)) {
       collectConstantCandidates(ConstCandMap, Inst, Idx);
     }
   } // end of for all operands
 }
 
 /// Collect all integer constants in the function that cannot be folded
 /// into an instruction itself.
 void ConstantHoistingPass::collectConstantCandidates(Function &Fn) {

llvm/lib/Transforms/Utils/Local.cpp

bool llvm::canReplaceOperandWithVariable(const Instruction *I, unsigned OpIdx) {
   // Early exit.
   if (!isa<Constant>(I->getOperand(OpIdx)))
     return true;
 
   switch (I->getOpcode()) {
   default:
     return true;
   case Instruction::Call:
-  case Instruction::Invoke:
+  case Instruction::Invoke: {
     // Can't handle inline asm. Skip it.
     if (isa<InlineAsm>(ImmutableCallSite(I).getCalledValue()))
       return false;
-    // Many arithmetic intrinsics have no issue taking a
-    // variable, however it's hard to distingish these from
-    // specials such as @llvm.frameaddress that require a constant.
-    if (isa<IntrinsicInst>(I))
+
+    ImmutableCallSite CS(I);
+
+    if (OpIdx < CS.getNumArgOperands()) {
+      // Intrinsics and calls that require a constant argument have that
+      // parameter annotated with ImmArg.
+      if (CS.paramHasAttr(OpIdx, Attribute::ImmArg))
         return false;
+    }
 
     // Constant bundle operands may need to retain their constant-ness for
     // correctness.
-    if (ImmutableCallSite(I).isBundleOperand(OpIdx))
+    if (CS.isBundleOperand(OpIdx))
       return false;
+
     return true;
+  }
   case Instruction::ShuffleVector:
     // Shufflevector masks are constant.
     return OpIdx != 2;
   case Instruction::Switch:
   case Instruction::ExtractValue:
     // All operands apart from the first are constant.
     return OpIdx == 0;
   case Instruction::InsertValue: