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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 2 13:19:55 PDT 2022


efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:1
+//===-- Arm64ECCallLowering.cpp - Lower indirect calls ----------*- C++ -*-===//
+//
----------------
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?


================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:73
+  Function *F =
+      Function::Create(Ty, GlobalValue::InternalLinkage, 0, "thunk", M);
+  F->setCallingConv(CallingConv::ARM64EC_Thunk_Native);
----------------
dpaoliello wrote:
> dpaoliello wrote:
> > There is a specific way that the names for these thunks are calculated - I'm working on getting this documented.
> Once we have the correct way to generate the names for thunks we may want to change the linkage to `WeakAnyLinkage`, since any thunk with the same name should be doing the same transformation to the parameters.
I assume you mean LinkOnceODRLinkage?

As a first cut, I think we should just use internal linkage, and not worry about using compatible mangling.


================
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) {
----------------
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?


================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:226
+
+bool Arm64ECCallLowering::runOnFunction(Function &F) {
+  SmallVector<CallBase *, 8> IndirectCalls;
----------------
dpaoliello wrote:
> My recommendation here is to change this into a `ModulePass`. You can then iterate over all blocks in all functions in the module and build a map of `FunctionType`->thunk, WITHOUT inserting those thunk `Function`s into the module (thus not invalidating the iterator).
> 
> Once you've completed this, iterate over the map and insert the thunks into the module.
> 
> I've made a "suggested edit" which is my work-in-progress for doing this, but please keep in mind that that 1) I haven't implemented the second step yet, 2) I'm not sure that `FunctionType` can be used as a key and 3) the way I'm dealing with CFG is by keeping the existing pass, and then fixing up the second argument in this pass to point to the thunk.
We can't use the FunctionType as a key, no.  At minimum, sret markings affect the calling convention.

Technically, we can add functions to the module while we're iterating over the function list, since it's a linked list, but it would probably be clearer to avoid that.


================
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
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:251-256
+      if (Function *F = CB->getCalledFunction()) {
+        if (F->isDSOLocal() || F->isIntrinsic())
+          continue;
+      }
+
+      IndirectCalls.push_back(CB);
----------------
dpaoliello wrote:
> This doesn't seem right: you need to rewrite the indirect calls AND generate thunks for any call to something that may be an x64 function.
> 
> Also, I'm pretty sure that `getCalledFunction` returns null for indirect calls.
Ideally, we do something like that, yes... but emitting the thunks is useless until we have some way to emit the hybmp tables.


================
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
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126811



More information about the llvm-commits mailing list