[PATCH] D154766: [GlobalISel] convergent intrinsics

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 3 inline comments as done.
sameerds 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 {
----------------
arsenm wrote:
> 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
I am not seeing an actionable comment here. The status before this change was that `getIntrinsicID()` is used in some places where the opcode is not a generic intrinsic, but the client still expects to find an `Intrinsic::ID` in the same place. Those places broke when I put an `isIntrinsic()` assert. This change does keeps the status quo. The only contribution here is an improvement in having a single predicate for generic intrinsic instead of checking four opcodes.


================
Comment at: llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:792
+  auto Attrs = Intrinsic::getAttributes(getContext(), ID);
+  auto MIB = buildInstr(
+      getIntrinsicCode(HasSideEffects, Attrs.hasFnAttr(Attribute::Convergent)));
----------------
arsenm wrote:
> I'd expect this to use another bool parameter along with HasSideEffects.
> 
> I'm also thinking ahead to noconvergent call sites
If you mean a parameter from the TableGen description, then this is the place where the `isConvergent` parameter from the def of the intrinsic gets translated to the `isConvergent` parameter in the def of `G_INTRINSIC_CONVERGENT_*`. The use of `getFnAttributes` on the intrinsic declaration is how we access the former, and the opcode selection inside `getIntrinsicCode` is how we set the latter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154766