GlobalISel: Don't fail translate on intrinsics with metadata

[paquette]

In rG55d10423a6a1f8286be05651b9c7143ef4e58b22

[GlobalISel] Don't translate intrinsics with metadata parameters.
Some intrinsics take metadata parameters. These all need custom handling of some form, and cannot possibly be lowered generically to G_INTRINSIC calls with vreg operands. Reject them, instead of hitting an assert later in getOrCreateVReg.

Is this still relevant? I guess we're just using custom legalization and selection to handle these nowadays?

(@ab do you have opinions here?)

[arsenm]

In D84276#2172675, @paquette wrote:

In rG55d10423a6a1f8286be05651b9c7143ef4e58b22

[GlobalISel] Don't translate intrinsics with metadata parameters.
Some intrinsics take metadata parameters. These all need custom handling of some form, and cannot possibly be lowered generically to G_INTRINSIC calls with vreg operands. Reject them, instead of hitting an assert later in getOrCreateVReg.

Is this still relevant? I guess we're just using custom legalization and selection to handle these nowadays?

(@ab do you have opinions here?)

I don’t think it makes any sense to try to use vregs for metadata. It’s like a weird form of immarg and shouldn’t be materialized. The generic intrinsics with metadata arguments should all probably be handled in the IRTranslator into G_* instructions with something else, but target intrinsics should just forward along the metadata operand

[ab]

In D84276#2172756, @arsenm wrote:

In D84276#2172675, @paquette wrote:

In rG55d10423a6a1f8286be05651b9c7143ef4e58b22

[GlobalISel] Don't translate intrinsics with metadata parameters.
Some intrinsics take metadata parameters. These all need custom handling of some form, and cannot possibly be lowered generically to G_INTRINSIC calls with vreg operands. Reject them, instead of hitting an assert later in getOrCreateVReg.

Is this still relevant? I guess we're just using custom legalization and selection to handle these nowadays?

(@ab do you have opinions here?)

This makes sense to me; for our targets we hadn't encountered many intrinsics with metadata, and I see that read/write_register have special opcodes now. If there are more for which it makes sense to deal with later, this seems totally reasonable. (but for read/write_register I could see them being emitted as the native copies in irtranslator directly, though I could see that either way)

At this point both of you have more relevant opinions than I do, but this LGTM ;)

I don’t think it makes any sense to try to use vregs for metadata. It’s like a weird form of immarg and shouldn’t be materialized. The generic intrinsics with metadata arguments should all probably be handled in the IRTranslator into G_* instructions with something else, but target intrinsics should just forward along the metadata operand

Agreed, this all makes sense to me

[arsenm]

In D84276#2176372, @ab wrote:

(but for read/write_register I could see them being emitted as the native copies in irtranslator directly, though I could see that either way)

I actually did this initially when I added support for these, but that doesn't work for arm for some reason

[arsenm]

Apparently the metadata can be not MDNode in the unhandled constrained FP intrinsics

[arsenm]

5f802be4e554f4eeb2c2c4137bed91b5b4ae740e

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

bool IRTranslator::translateCall(const User &U, MachineIRBuilder &MIRBuilder) {
   // Ignore the callsite attributes. Backend code is most likely not expecting
   // an intrinsic to sometimes have side effects and sometimes not.
   MachineInstrBuilder MIB =
       MIRBuilder.buildIntrinsic(ID, ResultRegs, !F->doesNotAccessMemory());
   if (isa<FPMathOperator>(CI))
     MIB->copyIRFlags(CI);
 
   for (auto &Arg : enumerate(CI.arg_operands())) {
-    // Some intrinsics take metadata parameters. Reject them.
-    if (isa<MetadataAsValue>(Arg.value()))
-      return false;
-
     // If this is required to be an immediate, don't materialize it in a
     // register.
     if (CI.paramHasAttr(Arg.index(), Attribute::ImmArg)) {
       if (ConstantInt *CI = dyn_cast<ConstantInt>(Arg.value())) {
         // imm arguments are more convenient than cimm (and realistically
         // probably sufficient), so use them.
         assert(CI->getBitWidth() <= 64 &&
                "large intrinsic immediates not handled");
         MIB.addImm(CI->getSExtValue());
       } else {
         MIB.addFPImm(cast<ConstantFP>(Arg.value()));
       }
+    } else if (auto MD = dyn_cast<MetadataAsValue>(Arg.value())) {
+      auto *MDN = dyn_cast<MDNode>(MD->getMetadata());
+      if (!MDN) // This was probably an MDString.
+        return false;
+      MIB.addMetadata(MDN);
     } else {
       ArrayRef<Register> VRegs = getOrCreateVRegs(*Arg.value());
       if (VRegs.size() > 1)
         return false;
       MIB.addUse(VRegs[0]);
     }
   }
 

llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll

+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -global-isel -march=amdgcn -mcpu=gfx900 -stop-after=irtranslator -o - %s | FileCheck %s
+; Make sure intrinsics with metadata arguments are translated
+
+define i32 @reloc_constant() {
+  ; CHECK-LABEL: name: reloc_constant
+  ; CHECK: bb.1 (%ir-block.0):
+  ; CHECK:   liveins: $sgpr30_sgpr31
+  ; CHECK:   [[COPY:%[0-9]+]]:sgpr_64 = COPY $sgpr30_sgpr31
+  ; CHECK:   [[INT:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.reloc.constant), !0
+  ; CHECK:   $vgpr0 = COPY [[INT]](s32)
+  ; CHECK:   [[COPY1:%[0-9]+]]:ccr_sgpr_64 = COPY [[COPY]]
+  ; CHECK:   S_SETPC_B64_return [[COPY1]], implicit $vgpr0
+  %val = call i32 @llvm.amdgcn.reloc.constant(metadata !0)
+  ret i32 %val
+}
+
+declare i32 @llvm.amdgcn.reloc.constant(metadata) #0
+
+attributes #0 = { nounwind readnone speculatable willreturn }
+
+!0 = !{!"arst"}