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

Tue Jun 27 00:31:07 PDT 2023

rovka 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:
> 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153761