[PATCH] D126811: [ARM64EC 10/?] Add support for lowering indirect calls.

[Daniel Paoliello via Phabricator via llvm-commits]

dpaoliello added inline comments.


================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:1
+//===-- Arm64ECCallLowering.cpp - Lower indirect calls ----------*- C++ -*-===//
+//
----------------
efriedma wrote:
> dpaoliello wrote:
> > Most other files in this directory are prefixed with `AArch64`, should we do the same with this one?
> What's the resulting name, though?  AArch64Arm64ECCallLowering.cpp?
Yeah, I'd go with AArch64Arm64ECCallLowering.cpp


================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:78
+  // Copy attributes to the thunk definition.
+  // FIXME: What do we need to copy other than sret?
+  for (unsigned i = 0; i < CB->arg_size(); ++i) {
----------------
efriedma wrote:
> dpaoliello wrote:
> > I would think that we'd need to copy anything that affects the calling convention.
> Right.
> 
> That said, I think for Windows x64/Arm64 ABI we don't normally use very many ABI attributes; I'm not sure we actually need to look at anything other than sret.  Maybe inreg?  Maybe also Swift ABI attributes, but I'm not going to worry about those for now.
> 
> I guess we could copy signext/zeroext/byval if we see them, somehow?
I think the only thing that we really need is byval.

It doesn't look like the x64 calling convention uses sret, so I'm not sure that it's needed.

signext and zeroext should have been taken care of by the caller (we don't need to redo that work in the thunk).

x64 calling convention also uses nest and swift, but I don't think we need them for now.


================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:242-250
+      // FIXME: isDSOLocal() doesn't do what we want; even if the symbol is
+      // technically local, automatic dllimport means the function it refers
+      // to might not be.
+      //
+      // FIXME: If a function is dllimport, we can just mark up the symbol
+      // using hybmp$x, and everything just works.  If the function is not
+      // marked dllimport, we can still mark up the symbol, but we somehow
----------------
efriedma wrote:
> dpaoliello wrote:
> > Would `isDeclaration` work?
> I think we also need to account for weak definitions, but we could do something like that, sure.
> 
> Ideally, we could distinguish between functions in the current library, vs. functions which are in other libraries.  For example, if the user is using -fvisibility.  But I guess it's not a big deal if we emit extra thunks.
Extra thunks should be dropped by the linker, so we should err on the side of more thunks: `isDefinition` is nice since it is one of the few cases where we know that no thunk is needed. We can always refine this later to reduce the number of useless thunks, but if we don't generate enough then we can't link.


================
Comment at: llvm/test/CodeGen/AArch64/arm64ec-cfg.ll:25-34
+; CHECK-NEXT:    str x30, [sp, #-16]! // 8-byte Folded Spill
+; CHECK-NEXT:    .seh_save_reg_x x30, 16
+; CHECK-NEXT:    .seh_endprologue
+; CHECK-NEXT:    adrp x8, __os_arm64x_check_icall
+; CHECK-NEXT:    adrp x10, thunk
+; CHECK-NEXT:    add x10, x10, :lo12:thunk
+; CHECK-NEXT:    mov x11, x0
----------------
efriedma wrote:
> dpaoliello wrote:
> > Yeah, this isn't right: indirect calls should be transformed like this, but direct calls shouldn't be changed. It's the linker's responsibility to rewrite direct calls (it will lookup the thunk by name).
> I'm not sure what isn't right here, specifically?
> 
> In this testcase, all the calls are indirect.  And we can't call `__os_arm64x_check_icall` directly, I think;  `__os_arm64x_check_icall` is the address of the function, not the function itself.
> 
> (I don't think there's any rule that says we *can't* transform direct calls this way, but probably we shouldn't.)
Ah, sorry, I misread the code. Yeah, this is correct - although it looks like we should add some direct-call cases then :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126811