[GlobalISel] GIntrinsic subclass to represent intrinsics in Generic Machine IR

[sameerds]

Summary

Some opcodes in generic MIR represent calls to intrinsics, where the intrinsic
ID is the first non-def operand to the instruction. These are now represented as
a subclass of GenericMachineInstr, and the method MachineInstr::getIntrinsicID()
is now moved to this subclass GIntrinsic.

Some target-defined instructions behave like GMIR intrinsics, and have an
Intrinsic::ID operand. But they should not be recognized as generic intrinsics,
and should not use GIntrinsic::getIntrinsicID(). Separated these out by
introducing a new AMDGPU::getIntrinsicID().

[arsenm]

llvm/include/llvm/CodeGen/MachineInstr.h

1945

I think making this an assert would be fine?

[sameerds]

llvm/include/llvm/CodeGen/MachineInstr.h

1944

@arsenm, do you see any value in introducing GIntrinsic as a subclass of GenericMachineInstr and moving getIntrinsicID() to it? I tried half of that in D154766, but I don't see any real advantage. I am thinking I'll roll it back in D154766.

1945

This allows to reduce verbosity at the client. For example, rather than doing isIntrinsic() { switch (ID) } you can just do switch (ID). Also instead of doing isIntrinsic() && getIntrinsicID() == XX you can simply do getIntrinsicID() == XX.

[arsenm]

llvm/include/llvm/CodeGen/MachineInstr.h

1944

We should probably do it and make more use of GenericMachineInstr

[sameerds]

• Introduce GIntrinsic subclass of GenericMachineInstr
• Moved MachineInstr::getIntrinsicID() to GIntrinsic

[sameerds]

llvm/include/llvm/CodeGen/MachineInstr.h

1944

Added the new GIntrinsic class and updated the review description.

[sameerds]

Bump! This is blocking further changes. Added some more reviewers to get more eyeballs.

[Pierre-vh]

(Adding my eyeballs to the list of eyeballs looking at this)

llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h

362 ↗

(On Diff #541843)

Very nitpicky, but if we're going down the route of using this class more, could we have some is method?
e.g. instead of

cast<GIntrinsic>(MI).getIntrinsicID() == Intrinsic::amdgcn_foo

have

cast<GIntrinsic>(MI).is(Intrinsic::amdgcn_foo)

Also maybe some method like hasSideEffect to check the opcode?

My point is that it's nice to have this type of class, but if we need to fallback to manual checking of the opcode/intrinsic id for any trivial check, this class is just adding verbosity to the code and doesn't add much value, IMO.

llvm/lib/Target/AMDGPU/AMDGPUInstrInfo.h

35–37

I would make the comment a bit more formal, maybe even use Doxygen and give some examples/context so we can easily then when to use this, and when to use GIntrinsic

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

13600 ↗

(On Diff #541843)

nit: does the LLVM coding style like auto if the type isn't present in the decl?
I love auto but I thought it was only recommended when we have something like cast/dyn_cast where the type is already spelled out on in the init

llvm/lib/Target/SPIRV/SPIRVUtils.cpp

232–234 ↗

(On Diff #541843)

[arsenm]

llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h

362 ↗

(On Diff #541843)

final?

364 ↗

(On Diff #541843)

can this be Intrinsic::ID

[sameerds]

• rebased
• added the GIntrinsic::is() method
• fixed issues with coding style

[sameerds]

llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h

362 ↗

(On Diff #541843)

Added both.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

13600 ↗

(On Diff #541843)

Good point. I am firmly in the "almost always auto" camp myself. But irrespective of everything else, it's generally a good idea to keep existing code as it is.

[vitalybuka]

llvm/include/llvm/CodeGen/MachineInstr.h

1935

does not compile
https://lab.llvm.org/buildbot/#/builders/70/builds/41248

[sameerds]

• reverted and fixed buildbot breakage
• there were dangling uses of MachineInstr::getIntrinsicID() in other targets
• note to self: don't forget to check Phab for a successful build before submitting!

[sameerds]

• fixed missing headers

llvm/include/llvm/CodeGen/MachineInstr.h

   /// Scan instructions immediately following MI and collect any matching
   /// DBG_VALUEs.
   void collectDebugValues(SmallVectorImpl<MachineInstr *> &DbgValues);
 
   /// Find all DBG_VALUEs that point to the register def in this instruction
   /// and point them to \p Reg instead.
   void changeDebugValuesDefReg(Register Reg);
 
-  /// Returns the Intrinsic::ID for this instruction.
-  /// \pre Must have an intrinsic ID operand.
+  bool isIntrinsic() const {
+    switch (getOpcode()) {
+    case TargetOpcode::G_INTRINSIC:
+    case TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS:
+      return true;
+    default:
+      return false;
+    }
+  }
+
+  /// Returns Intrinsic::ID if this is an intrinsic, else returns not_intrinsic.
   unsigned getIntrinsicID() const {
+    if (!isIntrinsic())
+      return Intrinsic::not_intrinsic;
     return getOperand(getNumExplicitDefs()).getIntrinsicID();
   }
 
   /// Sets all register debug operands in this debug value instruction to be
   /// undef.
   void setDebugValueUndef() {
     assert(isDebugValue() && "Must be a debug value instruction.");
     for (MachineOperand &MO : debug_operands()) {

llvm/lib/Target/AMDGPU/AMDGPUInstrInfo.h

 #define LLVM_LIB_TARGET_AMDGPU_AMDGPUINSTRINFO_H
 
 #include "Utils/AMDGPUBaseInfo.h"
 
 namespace llvm {
 
 class GCNSubtarget;
 class MachineMemOperand;
+class MachineInstr;
 
 class AMDGPUInstrInfo {
 public:
   explicit AMDGPUInstrInfo(const GCNSubtarget &st);
 
   static bool isUniformMMO(const MachineMemOperand *MMO);
 };
 
 namespace AMDGPU {
 
+// Return the intrinsic ID for some target-defined opcodes that behave like
+// intrinsics. Can't use MachineInstr::getIntrinsicID() since that expects an
+// actual generic instrinsic opcode.
+unsigned getIntrinsicID(const MachineInstr &I);
+
 struct RsrcIntrinsic {
   unsigned Intr;
   uint8_t RsrcArg;
   bool IsImage;
 };
 const RsrcIntrinsic *lookupRsrcIntrinsic(unsigned Intr);
 
 struct D16ImageDimIntrinsic {

llvm/lib/Target/AMDGPU/AMDGPUInstrInfo.cpp

 //===-- AMDGPUInstrInfo.cpp - Base class for AMD GPU InstrInfo ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file
 /// \brief Implementation of the TargetInstrInfo class that is common to all
 /// AMD GPUs.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AMDGPUInstrInfo.h"
 #include "AMDGPU.h"
+#include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Value.h"
 
 using namespace llvm;
 
 // Pin the vtable to this file.
 //void AMDGPUInstrInfo::anchor() {}
 
 AMDGPUInstrInfo::AMDGPUInstrInfo(const GCNSubtarget &ST) { }
 
+unsigned AMDGPU::getIntrinsicID(const MachineInstr &I) {
+  return I.getOperand(I.getNumExplicitDefs()).getIntrinsicID();
+}
 
 // TODO: Should largely merge with AMDGPUTTIImpl::isSourceOfDivergence.
 bool AMDGPUInstrInfo::isUniformMMO(const MachineMemOperand *MMO) {
   const Value *Ptr = MMO->getValue();
   // UndefValue means this is a load of a kernel input.  These are uniform.
   // Sometimes LDS instructions have constant pointers.
   // If Ptr is null, then that means this mem operand contains a
   // PseudoSourceValue like GOT.

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

bool AMDGPUInstructionSelector::select(MachineInstr &I) {
   case TargetOpcode::G_EXTRACT_VECTOR_ELT:
     return selectG_EXTRACT_VECTOR_ELT(I);
   case TargetOpcode::G_INSERT_VECTOR_ELT:
     return selectG_INSERT_VECTOR_ELT(I);
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_LOAD:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_LOAD_D16:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_STORE:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_STORE_D16: {
-    const AMDGPU::ImageDimIntrinsicInfo *Intr
-      = AMDGPU::getImageDimIntrinsicInfo(I.getIntrinsicID());
+    const AMDGPU::ImageDimIntrinsicInfo *Intr =
+        AMDGPU::getImageDimIntrinsicInfo(AMDGPU::getIntrinsicID(I));
     assert(Intr && "not an image intrinsic with image pseudo");
     return selectImageIntrinsic(I, Intr);
   }
   case AMDGPU::G_AMDGPU_INTRIN_BVH_INTERSECT_RAY:
     return selectBVHIntrinsic(I);
   case AMDGPU::G_SBFX:
   case AMDGPU::G_UBFX:
     return selectG_SBFX_UBFX(I);

llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp

case Intrinsic::amdgcn_ballot:
       break;
     }
     break;
   }
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_LOAD:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_LOAD_D16:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_STORE:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_STORE_D16: {
-    const AMDGPU::RsrcIntrinsic *RSrcIntrin
-      = AMDGPU::lookupRsrcIntrinsic(MI.getIntrinsicID());
+    const AMDGPU::RsrcIntrinsic *RSrcIntrin =
+        AMDGPU::lookupRsrcIntrinsic(AMDGPU::getIntrinsicID(MI));
     assert(RSrcIntrin && RSrcIntrin->IsImage);
     // Non-images can have complications from operands that allow both SGPR
     // and VGPR. For now it's too complicated to figure out the final opcode
     // to derive the register bank from the MCInstrDesc.
     applyMappingImage(MI, OpdMapper, MRI, RSrcIntrin->RsrcArg);
     return;
   }
   case AMDGPU::G_AMDGPU_INTRIN_BVH_INTERSECT_RAY: {
case AMDGPU::G_INTRINSIC: {
     }
     }
     break;
   }
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_LOAD:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_LOAD_D16:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_STORE:
   case AMDGPU::G_AMDGPU_INTRIN_IMAGE_STORE_D16: {
-    auto IntrID = MI.getIntrinsicID();
+    auto IntrID = AMDGPU::getIntrinsicID(MI);
     const AMDGPU::RsrcIntrinsic *RSrcIntrin = AMDGPU::lookupRsrcIntrinsic(IntrID);
     assert(RSrcIntrin && "missing RsrcIntrinsic for image intrinsic");
     // Non-images can have complications from operands that allow both SGPR
     // and VGPR. For now it's too complicated to figure out the final opcode
     // to derive the register bank from the MCInstrDesc.
     assert(RSrcIntrin->IsImage);
     return getImageMapping(MRI, MI, RSrcIntrin->RsrcArg);
   }