[PATCH] D154766: [GlobalISel] convergent intrinsics
Jay Foad via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 27 03:33:47 PDT 2023
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
More information about the llvm-commits
mailing list