[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++ -*-===//
+//
----------------
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