[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.
dpaoliello added a comment. @efriedma the patch is failing to apply - can you please update your baseline, or move this to a GitHub PR? 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
[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.
dpaoliello added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64CallingConvention.td:248-257 + // The first 4 FP/Vector arguments are passed in XMM registers. + CCIfType<[f16], + CCAssignToRegWithShadow<[H0, H1, H2, H3], + [X0, X1, X2, X2]>>, + CCIfType<[f32], + CCAssignToRegWithShadow<[S0, S1, S2, S3], + [X0, X1, X2, X2]>>, These seem wrong, shouldn't the shadows be `[X0, X1, X2, X3]` (instead of `X2` twice) 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
[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.
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
[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.
dpaoliello added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64CallingConvention.td:555-557 +def CSR_Win_AArch64_Arm64EC_Thunk : CalleeSavedRegs<(add X19, X20, X21, X22, X23, X24, + X25, X26, X27, X28, FP, LR, + (sequence "Q%u", 6, 15))>; I've been hitting asserts in `computeCalleeSaveRegisterPairs` due to this: LLVM doesn't support gaps when saving registers for Windows (~line 2744 of AArch64FrameLowering.cpp), so placing the smaller registers before the larger ones causes issues if those smaller ones aren't paired (the `assert(OffsetPre % Scale == 0);` fires since `OffsetPre` will be a multiple of 8 but `Scale` will be 16). 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
[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.
dpaoliello added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64MCInstLower.cpp:37-38 MCSymbol * AArch64MCInstLower::GetGlobalAddressSymbol(const MachineOperand &MO) const { const GlobalValue *GV = MO.getGlobal(); We now also need to call this for constants in case the function is only referenced from a variable (e.g., a VTable) and is never called elsewhere. I split this function into a new function `GetGlobalValueSymbol`: ``` MCSymbol * AArch64MCInstLower::GetGlobalAddressSymbol(const MachineOperand &MO) const { return GetGlobalValueSymbol(MO.getGlobal(), MO.getTargetFlags()); } MCSymbol * AArch64MCInstLower::GetGlobalValueSymbol(const GlobalValue *GV, unsigned TargetFlags) const { ... } ``` And then called it from `AArch64AsmPrinter`: ``` const MCExpr *AArch64AsmPrinter::lowerConstant(const Constant *CV) { if (const GlobalValue *GV = dyn_cast(CV)) return MCSymbolRefExpr::create(MCInstLowering.GetGlobalValueSymbol(GV, 0), OutContext); return AsmPrinter::lowerConstant(CV); } ``` 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
[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.
dpaoliello added a comment. Another issue: looks like the names in `AArch64Subtarget.h` need to be fully decorated, including the leading `#` - so `#__chkstk_arm64ec` and `#__security_check_cookie_arm64ec`. 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
[PATCH] D43002: [CodeView] Emit S_OBJNAME record
dpaoliello added a comment. Herald added a subscriber: ormris. @rnk @aganea Ping... Just wanted to see what was happening with this change, and if there is anything I can do to help get it merged in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D43002/new/ https://reviews.llvm.org/D43002 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits