[PATCH] D126811: [ARM64EC 10/?] Add support for lowering indirect calls.
Daniel Paoliello via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 2 12:29:05 PDT 2022
dpaoliello added inline comments.
================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:1
+//===-- Arm64ECCallLowering.cpp - Lower indirect calls ----------*- C++ -*-===//
+//
----------------
Most other files in this directory are prefixed with `AArch64`, should we do the same with this one?
================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:72-73
+ FunctionType *Ty = FunctionType::get(RetTy, DefArgTypes, false);
+ Function *F =
+ Function::Create(Ty, GlobalValue::InternalLinkage, 0, "thunk", M);
+ F->setCallingConv(CallingConv::ARM64EC_Thunk_Native);
----------------
I didn't think that a `FunctionPass` was allowed to modify the list of functions - aren't we currently iterating through the list of function at this point (and so a realloc would invalidate the iterator)?
================
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);
----------------
There is a specific way that the names for these thunks are calculated - I'm working on getting this documented.
================
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:
> 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.
================
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) {
----------------
I would think that we'd need to copy anything that affects the calling convention.
================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:140
+ }
+ // FIXME: Transfer necessary attributes? sret? anything else?
+ // FIXME: Try to share thunks. This probably involves simplifying the
----------------
Again, probably only things that affect the calling convention...
================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:141-142
+ // FIXME: Transfer necessary attributes? sret? anything else?
+ // FIXME: Try to share thunks. This probably involves simplifying the
+ // argument types (translating all integers/pointers to i64, etc.)
+ auto *CallTy = FunctionType::get(X64RetType, ArgTypes, false);
----------------
The way that I was going to try to share thunks is by creating a map of `FunctionType`->thunk `Function`. This also solves the issue of modifying the list of functions in a module while iterating over it: you can create the `Function` while iterating WITHOUT adding it to the module, and then iterate the map afterwards and add each thunk `Function` to the module.
================
Comment at: llvm/lib/Target/AArch64/Arm64ECCallLowering.cpp:226
+
+bool Arm64ECCallLowering::runOnFunction(Function &F) {
+ SmallVector<CallBase *, 8> IndirectCalls;
----------------
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.
================
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
----------------
Would `isDeclaration` work?
================
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);
----------------
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.
================
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
----------------
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).
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