dpaoliello added inline comments.
================ Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2726-2729 + // FIXME: For non-dllimport functions, MSVC emits the same entry + // twice, for reasons I don't understand. I have to assume the linker + // ignores the redundant entry; there aren't any reasonable semantics + // to attach to it. ---------------- To be clear: you're seeing MSVC emit the same entry twice, but you opted to emit the entry only once? ================ Comment at: llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp:332-333 + } + // FIXME: Copy anything other than sret? Shouldn't be necessary for normal + // C ABI, but might show up in other cases. + BasicBlock *BB = BasicBlock::Create(M->getContext(), "", F); ---------------- Having a look through the available parameter attributes: * Do we need `byval` and `byref`, or are they irrelevant to the thunk? * Should we copy the alignment attributes to keep them the same? * Might be interesting to copy some of the optimization related ones (unless we're too late in the compilation) like `readnone`, `readonly`, `immarg`, `returned`, etc. ================ Comment at: llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp:608 + // + // FIXME: Handle functions with weak linkage? + if (F.hasExternalLinkage() || F.hasWeakLinkage() || F.hasLinkOnceLinkage()) { ---------------- Are you asking if we should be handling functions with weak linkage, or is this a TODO to implement support somewhere else? ================ Comment at: llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp:641-645 + // 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 + // need an extra stub to compute the correct callee. Not really + // understanding how this works. ---------------- Can you provide some more details about this? What extra stub are you seeing? ================ Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1618 + if (Subtarget->isWindowsArm64EC()) { + // FIXME: are there other intrinsics we need to add here? + setLibcallName(RTLIB::MEMCPY, "#memcpy"); ---------------- Full list is: ``` #ceil #floor #memcmp #memcpy #memmove #memset ``` ================ Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:4616-4617 def SEH_PACSignLR : Pseudo<(outs), (ins), []>, Sched<[]>; + def SEH_SaveAnyRegQP : Pseudo<(outs), (ins i32imm:$reg0, i32imm:$reg1, i32imm:$offs), []>, Sched<[]>; + def SEH_SaveAnyRegQPX : Pseudo<(outs), (ins i32imm:$reg0, i32imm:$reg1, i32imm:$offs), []>, Sched<[]>; } ---------------- Should these be added to `AArch64InstrInfo::isSEHInstruction`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157547/new/ https://reviews.llvm.org/D157547 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits