[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 {
----------------
sameerds wrote:
> 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.
Targets probably shouldn't define their own G_INTRINSIC instructions. They can define their own G_* instructions, but seems unnecessary they would need the abstraction layer to track different intrinsics in them


================
Comment at: llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:792
+  auto Attrs = Intrinsic::getAttributes(getContext(), ID);
+  auto MIB = buildInstr(
+      getIntrinsicCode(HasSideEffects, Attrs.hasFnAttr(Attribute::Convergent)));
----------------
I'd expect this to use another bool parameter along with HasSideEffects.

I'm also thinking ahead to noconvergent call sites


================
Comment at: llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:802
                                                      ArrayRef<DstOp> Results,
                                                      bool HasSideEffects) {
+  auto Attrs = Intrinsic::getAttributes(getContext(), ID);
----------------
Ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154766