[PATCH] D67396: [ConstantHoisting] Do not attempt to hoist ImmArgs

[Sam Elliott via Phabricator via llvm-commits]

lenary created this revision.
lenary added reviewers: aoli, jmolloy, arsenm.
Herald added subscribers: llvm-commits, s.egerton, PkmX, simoncook, hiraditya, wdng.
Herald added a project: LLVM.

`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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67396

Files:
  llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
  llvm/lib/Transforms/Utils/Local.cpp


Index: llvm/lib/Transforms/Utils/Local.cpp
===================================================================
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -2917,21 +2917,27 @@
   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))
-      return false;
+
+    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;
Index: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -491,7 +491,7 @@
     // `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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D67396.219535.patch
Type: text/x-patch
Size: 2032 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190910/b91f393e/attachment.bin>