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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 6 inline comments as done.
sameerds added inline comments.


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


================
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();
----------------
Pierre-vh wrote:
> 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
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155556