[PATCH] D154766: [GlobalISel] convergent intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
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 {
----------------
I don't think we should spread using intrinsic operands for anything else? I'd hope the verifier didn't allow this


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:1519
       }
       if (!NoSideEffects && !DeclHasSideEffects) {
         report("G_INTRINSIC_W_SIDE_EFFECTS used with readnone intrinsic", MI);
----------------
Should implement the matching convergent intrinsic declaration checks


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:601
   if (!I.getNumMemOperands()) {
-    assert(I.getOpcode() == TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS);
+    assert(I.isIntrinsic());
     addMemoryOperands(I.getOperand(2 + OpOffset).getImm(), MIB);
----------------
This is more permissive than before


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154766