[PATCH] D153761: [AMDGPU] ISel for @llvm.amdgcn.cs.chain intrinsic

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2517
+  if (!F || !F->isIntrinsic() || ID == Intrinsic::not_intrinsic ||
+      ID == Intrinsic::amdgcn_cs_chain)
     return translateCallBase(CI, MIRBuilder);
----------------
arsenm wrote:
> rovka wrote:
> > arsenm wrote:
> > > rovka wrote:
> > > > arsenm wrote:
> > > > > Probably should avoid touching the generic IRTranslator. Can you just handle this as a legalize on the intrinsic itself? Fundamentally this isn't really any different from emitting a libcall, you'd just need to access the CallLowering from the legalize function
> > > > Yeah, I'm not a fan of this either, but the IRTranslator already splits up aggregates and then errors out somewhere along the way. Even if I somehow fix that error, I think it will be more of a maintenance burden to try to hold on to these aggregates, and if we don't keep them together long enough then we won't know which values are meant to go in SGPRs and which in VGPRs. It's clearer and more straightforward to deal with this early on.
> > > > 
> > > > But I do agree that this code doesn't look great. I can think of a couple of ways to make it a bit better:
> > > > 1. Move the check into translateKnownIntrinsic, which already goes through a lot of intrinsics. At the moment none of them are target-specific, but that might change in the future anyway (SelectionDAG has at least 2 aarch64 intrinsics in similar generic code, I haven't checked if GlobalISel supports them yet), or
> > > > 2. Add an `IsCall` intrinsic attribute and pass intrinsics with that attribute through CallLowering (either `lowerCall` directly or a new hook, e.g. `lowerIntrinsicAsCall`)
> > > > 
> > > > Do either of those sound more palatable?
> > > Why do you need to preserve the aggregates? What's the error? I would expect you don't care about aggregates to lower this, and that aggregates should just work
> > I care about the aggregates because otherwise how would I know which values to put in SGPRs and which ones to put in VGPRs? The semantics of the intrinsic are that the 3rd argument contains the stuff that goes into SGPRs, and the 4th contains the stuff that goes into VGPRs. If I allow these to be broken up before legalization (or whenever we decide to actually lower this), then I'll end up with a `G_INTRINSIC_WITH_SIDE_EFFECTS` with a whole lot of smaller registers, and I won't know where the ones from the first aggregate end and the ones from the second aggregate begin, aka I won't know which ones I should copy to SGPRs and which ones to VGPRs. Does that make sense? 
> > 
> > (The actual error is IIRC [[ https://github.com/llvm/llvm-project/blob/f22af204edfd1a8f16511b2635ed41c00fc5502f/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp#L2560 | here ]], but I don't think that's the most important point; I could fix that to build a merge, like SelectionDAGBuilder does - but even in DAGISel we lose the merge in the first round of combines, and with it the information on what kind of reg each value should be placed in).
> I would have assumed this is indicated by using inreg on the call parameters. If that were the case, I suppose you would still need to find a way to preserve that information
An alternative scheme would be you could just have an immarg parameter that indicates which operand index the VGPRs (start from 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7378
+    assert(I.arg_size() == 5 && "Additional args not supported yet");
+    assert(dyn_cast<ConstantInt>(I.getOperand(4))->isZero() &&
+           "Non-zero flags not supported yet");
----------------
unchecked dyn_cast in assert, should use cast or precheck with isa


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153761