[PATCH] D154766: [GlobalISel] convergent intrinsics

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked an inline comment as done.
sameerds added inline comments.
Herald added a subscriber: pmatos.


================
Comment at: llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:792
+  auto Attrs = Intrinsic::getAttributes(getContext(), ID);
+  auto MIB = buildInstr(
+      getIntrinsicCode(HasSideEffects, Attrs.hasFnAttr(Attribute::Convergent)));
----------------
sameerds wrote:
> arsenm wrote:
> > sameerds wrote:
> > > 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.
> > I mean buildIntrinsic has a HasSideEffects bool parameter. I expect another partner operand for convergent, rather than looking it up through the declaration here. A different buildIntrinsic overload that queries the underlying declaration is potentially useful, but I don't want to mix the two here
> Fixed this by introducing an explicit version that takes two boolean arguments. The default version determines both parameters (side-effects and convergent) from the intrinsic ID, and then passes that to the explicit version. I believe this is a strong way to lay out what is intended.
Bump!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154766