[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

2023-09-26 Thread Daniel Paoliello via Phabricator via cfe-commits
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.

2023-10-02 Thread Daniel Paoliello via Phabricator via cfe-commits
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.

2023-08-17 Thread Daniel Paoliello via Phabricator via cfe-commits
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.

2023-08-22 Thread Daniel Paoliello via Phabricator via cfe-commits
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.

2023-09-13 Thread Daniel Paoliello via Phabricator via cfe-commits
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.

2023-08-23 Thread Daniel Paoliello via Phabricator via cfe-commits
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

2021-08-30 Thread Daniel Paoliello via Phabricator via cfe-commits
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