[PATCH] D154766: [GlobalISel] convergent intrinsics

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:1111
 
   /// Build and insert either a G_INTRINSIC (if \p HasSideEffects is false) or
   /// G_INTRINSIC_W_SIDE_EFFECTS instruction. Its first operand will be the
----------------
Comment needs to explain what the non-"Explicit" ones do.

Do you actually need the `Explicit` suffix? You could just leave them all as different overloads of `buildIntrinsic`.


================
Comment at: llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:778
 
-MachineInstrBuilder MachineIRBuilder::buildIntrinsic(Intrinsic::ID ID,
-                                                     ArrayRef<Register> ResultRegs,
-                                                     bool HasSideEffects) {
-  auto MIB =
-      buildInstr(HasSideEffects ? TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS
-                                : TargetOpcode::G_INTRINSIC);
+static unsigned getIntrinsicCode(bool HasSideEffects, bool IsConvergent) {
+  if (HasSideEffects && IsConvergent)
----------------
s/Code/Opcode/


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:228-229
+
+    bool verifyGIntrinsicWithSideEffects(const MachineInstr *MI);
+    bool verifyConvergentGIntrinsic(const MachineInstr *MI);
     void verifyPreISelGenericInstruction(const MachineInstr *MI);
----------------
I find these names weird, because verifyGIntrinsicWithSideEffects does not just verify G_INTRINSIC_W_SIDE_EFFECTS. It verifies the side-effect-ness of all intrinsics.

Maybe rename to:
verifyGIntrinsicSideEffects
verifyGIntrinsicConvergence
?

Or just re-inline them back into their only caller?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154766