[PATCH] D154766: [GlobalISel] convergent intrinsics

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:859
 
-G_INTRINSIC, G_INTRINSIC_W_SIDE_EFFECTS
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+G_INTRINSIC and variants
+^^^^^^^^^^^^^^^^^^^^^^^^
----------------
tschuett wrote:
> Could you please out all four of them? It is easier to read for me.
Trying a different way to mention the variants. Hope this one makes sense!


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1373-1383
+  bool isIntrinsic() const {
+    switch (getOpcode()) {
+    case TargetOpcode::G_INTRINSIC:
+    case TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS:
+    case TargetOpcode::G_INTRINSIC_CONVERGENT:
+    case TargetOpcode::G_INTRINSIC_CONVERGENT_W_SIDE_EFFECTS:
+      return true;
----------------
yassingh wrote:
> Can this fit in GenericMachineInstrs.h? Also the name can be modified a bit to reflect it only queries generic instructions.
Nice catch! This also provided a way to resolve other comments about `isIntrinsic()`.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1948
   /// Returns the Intrinsic::ID for this instruction.
-  /// \pre Must have an intrinsic ID operand.
+  /// \pre Must have an intrinsic ID operand (not necessarily isIntrinsic())
   unsigned getIntrinsicID() const {
----------------
arsenm wrote:
> I don't think we should spread using intrinsic operands for anything else? I'd hope the verifier didn't allow this
This comment was tersely trying to say that `isIntrinsic()` may not capture all the generic intrinsics, such as target-defined G_INTRINSIC_* opcodes. But the latest version now drops introducing `isIntrinsic()` and instead introduces `isa<GIntrinsic>()` which is easier to describe.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:1520
       if (!NoSideEffects && !DeclHasSideEffects) {
         report("G_INTRINSIC_W_SIDE_EFFECTS used with readnone intrinsic", MI);
         break;
----------------
yassingh wrote:
> Will these error messages suffice for CONVERGENT intrinsics too? 
Fixed it by printing the actual name of the intrinsic.


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:860
     bool IsIntrinsic =
         SrcGIOrNull->TheDef->getName() == "G_INTRINSIC" ||
+        SrcGIOrNull->TheDef->getName() == "G_INTRINSIC_W_SIDE_EFFECTS" ||
----------------
arsenm wrote:
> .startswith("G_INTRINSIC")?
I tried that, but it fails with other opcodes that start with G_INTRINSIC but are not really intrinsics. That change needs to happen separately, and can include this cleanup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154766