[PATCH] D94597: [X86] Lower calls with clang.arc.attachedcall bundle

Tue May 11 10:12:52 PDT 2021

ab accepted this revision.
ab added a comment.
This revision is now accepted and ready to land.

Ah sorry, I didn't see you tried that already!  I don't think we want to weaken the verifier check, this is probably the only case where it makes sense to take the address.  I guess we've meandered through the same decision process, thanks for the patience ;)  But it might be worth explaining these decisions in the commit message.  Also in the langref paragraph for the bundle, it says the call is always emitted for the bundle.
Which brings another idea: how about emitting the call when lowering the bundle on AArch64 as well?  There's not much room for better regalloc or other optimizations in the sequence anyway, right?

But back to x86: there's a couple things we could do to make the behavior more obvious (split the bundle into distinct retain/claim ones?  replace the `sel` imm MO in these pseudos with an actual GV/ES reference to the symbol?).  But I'm not sure that makes a real difference, so LGTM as-is

================
Comment at: llvm/lib/Target/X86/X86ExpandPseudo.cpp:244
+  auto *I8PtrTy = PointerType::get(IntegerType::get(Context, 8), 0);
+  FunctionCallee FCache = M->getOrInsertFunction(
+      RuntimeCallType == 0 ? "objc_retainAutoreleasedReturnValue"
----------------
why "Cache"?

================
Comment at: llvm/test/CodeGen/X86/call-rv-marker.ll:161

+; TODO: This should use "callq *_fptr(%rip)".
 define i8* @rv_marker_5_indirect_call() {
----------------
Hmm, I'm curious, do you know why this doesn't trigger?  Is it just missing entries in the fold tables?  (does the extra operand make that trickier than it should be?)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94597