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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added a comment.

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



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h:362
+/// Represents a call to an intrinsic.
+class GIntrinsic : public GenericMachineInstr {
+public:
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstrInfo.h:35-37
+// 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.
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:13600
     // site specifies a lower alignment?
-    Intrinsic::ID IID = MI->getIntrinsicID();
+    auto IID = GI->getIntrinsicID();
     LLVMContext &Ctx = KB.getMachineFunction().getFunction().getContext();
----------------
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


================
Comment at: llvm/lib/Target/SPIRV/SPIRVUtils.cpp:232-234
+  if (auto *GI = dyn_cast<GIntrinsic>(&MI)) {
+    return GI->getIntrinsicID() == IntrinsicID;
+  }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155556/new/

https://reviews.llvm.org/D155556