[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-13 Thread Kerry McLaughlin via cfe-commits

https://github.com/kmclaughlin-arm closed 
https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-13 Thread Kerry McLaughlin via cfe-commits


@@ -214,7 +232,8 @@ declare double @za_shared_callee(double) "aarch64_inout_za"
 define double  @za_new_caller_to_za_shared_callee(double %x) nounwind noinline 
optnone "aarch64_new_za"{
 ; CHECK-COMMON-LABEL: za_new_caller_to_za_shared_callee:
 ; CHECK-COMMON:   // %bb.0: // %prelude
-; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-16]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-32]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:str x19, [sp, #16] // 8-byte Folded Spill

kmclaughlin-arm wrote:

I've taken a closer look at this and I think the changes in 
`@za_new_caller_to_za_shared_callee` are correct. Because it's an 
`aarch64_new_za` function, it has to set up a lazy-save buffer on entry and a 
variable sized object is allocated on the stack for this. The function is not 
streaming, so x19 will only be spilled if the function has SVE.

The buffer is not used however, and 
https://github.com/llvm/llvm-project/pull/81648 was created to remove the 
lazy-save from tests like this where it is not required.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-13 Thread Sander de Smalen via cfe-commits


@@ -2995,7 +3062,9 @@ bool AArch64FrameLowering::spillCalleeSavedRegisters(
 ArrayRef CSI, const TargetRegisterInfo *TRI) const {
   MachineFunction &MF = *MBB.getParent();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
+  AArch64FunctionInfo *AFI = MF.getInfo();
   bool NeedsWinCFI = needsWinCFI(MF);
+  bool HasSVE = MF.getSubtarget().hasSVE();

sdesmalen-arm wrote:

nit: this only has one use, which seems quite far away from the definition. 
Perhaps just inline it?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-13 Thread Sander de Smalen via cfe-commits

https://github.com/sdesmalen-arm edited 
https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-13 Thread Sander de Smalen via cfe-commits


@@ -3062,7 +3131,68 @@ bool AArch64FrameLowering::spillCalleeSavedRegisters(
Size = 2;
Alignment = Align(2);
break;
+case RegPairInfo::VG:
+  StrOpc = AArch64::STRXui;
+  Size = 8;
+  Alignment = Align(8);
+  break;
+}
+
+unsigned X0Scratch = AArch64::NoRegister;
+if (Reg1 == AArch64::VG) {
+  // Find an available register to store value of VG to.
+  Reg1 = findScratchNonCalleeSaveRegister(&MBB);
+  assert(Reg1 != AArch64::NoRegister);
+  SMEAttrs Attrs(MF.getFunction());
+
+  if (Attrs.hasStreamingBody() && !Attrs.hasStreamingInterface() &&
+  AFI->getStreamingVGIdx() == std::numeric_limits::max()) {
+// For locally-streaming functions, we need to store both the streaming
+// & non-streaming VG. Spill the streaming value first.
+BuildMI(MBB, MI, DL, TII.get(AArch64::RDSVLI_XI), Reg1)
+.addImm(1)
+.setMIFlag(MachineInstr::FrameSetup);
+BuildMI(MBB, MI, DL, TII.get(AArch64::UBFMXri), Reg1)
+.addReg(Reg1)
+.addImm(3)
+.addImm(63)
+.setMIFlag(MachineInstr::FrameSetup);
+
+AFI->setStreamingVGIdx(RPI.FrameIdx);
+  } else {
+if (HasSVE)
+  BuildMI(MBB, MI, DL, TII.get(AArch64::CNTD_XPiI), Reg1)
+  .addImm(31)
+  .addImm(1)
+  .setMIFlag(MachineInstr::FrameSetup);
+else {
+  const AArch64Subtarget &STI = MF.getSubtarget();
+  for (const auto &LiveIn : MBB.liveins())
+if (STI.getRegisterInfo()->isSuperOrSubRegisterEq(AArch64::X0,
+  LiveIn.PhysReg))
+  X0Scratch = Reg1;
+
+  if (X0Scratch != AArch64::NoRegister)
+BuildMI(MBB, MI, DL, TII.get(AArch64::ORRXrr), Reg1)
+.addReg(AArch64::XZR)
+.addReg(AArch64::X0, RegState::Undef)
+.addReg(AArch64::X0, RegState::Implicit)
+.setMIFlag(MachineInstr::FrameSetup);
+
+  const uint32_t *RegMask = TRI->getCallPreservedMask(
+  MF, CallingConv::
+  AArch64_SME_ABI_Support_Routines_PreserveMost_From_X1);
+  BuildMI(MBB, MI, DL, TII.get(AArch64::BL))
+  .addExternalSymbol("__arm_get_current_vg")
+  .addRegMask(RegMask)
+  .addReg(AArch64::X0, RegState::ImplicitDefine)
+  .setMIFlag(MachineInstr::FrameSetup);
+  Reg1 = AArch64::X0;
+}
+AFI->setVGIdx(RPI.FrameIdx);
+  }

sdesmalen-arm wrote:

nit: put `AFI->setVGIdx(RPI.FrameIdx);` in both the `if(HasSVE)` and the `else` 
branch, and remove a level of indentation?

```suggestion
  } else if (HasSVE) {
BuildMI(MBB, MI, DL, TII.get(AArch64::CNTD_XPiI), Reg1)
.addImm(31)
.addImm(1)
.setMIFlag(MachineInstr::FrameSetup);
AFI->setVGIdx(RPI.FrameIdx);
  } else { 
const AArch64Subtarget &STI = MF.getSubtarget();
for (const auto &LiveIn : MBB.liveins())
  if (STI.getRegisterInfo()->isSuperOrSubRegisterEq(AArch64::X0,
LiveIn.PhysReg))
X0Scratch = Reg1;
  
if (X0Scratch != AArch64::NoRegister)
  BuildMI(MBB, MI, DL, TII.get(AArch64::ORRXrr), Reg1)
  .addReg(AArch64::XZR)
  .addReg(AArch64::X0, RegState::Undef)
  .addReg(AArch64::X0, RegState::Implicit)
  .setMIFlag(MachineInstr::FrameSetup);

const uint32_t *RegMask = TRI->getCallPreservedMask(
MF, CallingConv::
AArch64_SME_ABI_Support_Routines_PreserveMost_From_X1);
BuildMI(MBB, MI, DL, TII.get(AArch64::BL))
.addExternalSymbol("__arm_get_current_vg")
.addRegMask(RegMask)
.addReg(AArch64::X0, RegState::ImplicitDefine)
.setMIFlag(MachineInstr::FrameSetup);
Reg1 = AArch64::X0;
AFI->setVGIdx(RPI.FrameIdx);
  }
```

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-13 Thread Sander de Smalen via cfe-commits


@@ -3062,7 +3131,68 @@ bool AArch64FrameLowering::spillCalleeSavedRegisters(
Size = 2;
Alignment = Align(2);
break;
+case RegPairInfo::VG:
+  StrOpc = AArch64::STRXui;
+  Size = 8;
+  Alignment = Align(8);
+  break;
+}
+
+unsigned X0Scratch = AArch64::NoRegister;
+if (Reg1 == AArch64::VG) {
+  // Find an available register to store value of VG to.
+  Reg1 = findScratchNonCalleeSaveRegister(&MBB);
+  assert(Reg1 != AArch64::NoRegister);
+  SMEAttrs Attrs(MF.getFunction());
+
+  if (Attrs.hasStreamingBody() && !Attrs.hasStreamingInterface() &&
+  AFI->getStreamingVGIdx() == std::numeric_limits::max()) {
+// For locally-streaming functions, we need to store both the streaming
+// & non-streaming VG. Spill the streaming value first.
+BuildMI(MBB, MI, DL, TII.get(AArch64::RDSVLI_XI), Reg1)
+.addImm(1)
+.setMIFlag(MachineInstr::FrameSetup);
+BuildMI(MBB, MI, DL, TII.get(AArch64::UBFMXri), Reg1)
+.addReg(Reg1)
+.addImm(3)
+.addImm(63)
+.setMIFlag(MachineInstr::FrameSetup);
+
+AFI->setStreamingVGIdx(RPI.FrameIdx);
+  } else {
+if (HasSVE)
+  BuildMI(MBB, MI, DL, TII.get(AArch64::CNTD_XPiI), Reg1)
+  .addImm(31)
+  .addImm(1)
+  .setMIFlag(MachineInstr::FrameSetup);
+else {
+  const AArch64Subtarget &STI = MF.getSubtarget();
+  for (const auto &LiveIn : MBB.liveins())
+if (STI.getRegisterInfo()->isSuperOrSubRegisterEq(AArch64::X0,
+  LiveIn.PhysReg))
+  X0Scratch = Reg1;

sdesmalen-arm wrote:

Is this missing a `break` ?

If so, you can write this using any_of, e.g.:
```
if (llvm::any_of(MBB.liveins(), [&STI](const RegisterMaskPair &LiveIn) {
  return STI.getRegisterInfo()->isSuperOrSubRegisterEq(AArch64::X0,
   LiveIn.PhysReg);
}))
  X0Scratch = Reg1;
```

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-13 Thread Sander de Smalen via cfe-commits

https://github.com/sdesmalen-arm approved this pull request.

Some very minor nits, but overall LGTM.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-11 Thread Eli Friedman via cfe-commits

https://github.com/efriedma-quic approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-11 Thread Kerry McLaughlin via cfe-commits


@@ -196,12 +196,14 @@ bool AArch64FunctionInfo::needsAsyncDwarfUnwindInfo(
 const MachineFunction &MF) const {
   if (!NeedsAsyncDwarfUnwindInfo) {
 const Function &F = MF.getFunction();
+const AArch64FunctionInfo *AFI = MF.getInfo();
 //  The check got "minsize" is because epilogue unwind info is not emitted
 //  (yet) for homogeneous epilogues, outlined functions, and functions
 //  outlined from.
-NeedsAsyncDwarfUnwindInfo = needsDwarfUnwindInfo(MF) &&
-F.getUWTableKind() == UWTableKind::Async &&
-!F.hasMinSize();
+NeedsAsyncDwarfUnwindInfo =
+(needsDwarfUnwindInfo(MF) && F.getUWTableKind() == UWTableKind::Async 
&&

kmclaughlin-arm wrote:

I think outlining from functions with streaming-mode changes needs more 
investigation. I don't think this is just a concern for async unwind; I noticed 
that when passing `-enable-machine-outliner` to sme-vg-to-stack.ll that some 
calls are outlined with only one of the smstart/smstop instructions surrounding 
the call. I'm not sure if this is safe yet, so for now I've disabled outlining 
for these functions in `isFunctionSafeToOutlineFrom`.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-10 Thread Eli Friedman via cfe-commits


@@ -196,12 +196,14 @@ bool AArch64FunctionInfo::needsAsyncDwarfUnwindInfo(
 const MachineFunction &MF) const {
   if (!NeedsAsyncDwarfUnwindInfo) {
 const Function &F = MF.getFunction();
+const AArch64FunctionInfo *AFI = MF.getInfo();
 //  The check got "minsize" is because epilogue unwind info is not emitted
 //  (yet) for homogeneous epilogues, outlined functions, and functions
 //  outlined from.
-NeedsAsyncDwarfUnwindInfo = needsDwarfUnwindInfo(MF) &&
-F.getUWTableKind() == UWTableKind::Async &&
-!F.hasMinSize();
+NeedsAsyncDwarfUnwindInfo =
+(needsDwarfUnwindInfo(MF) && F.getUWTableKind() == UWTableKind::Async 
&&

efriedma-quic wrote:

I'm not sure exactly what the issues are with outlining at this point, but the 
last time async-unwind was looked at, there apparently were issues.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-10 Thread Eli Friedman via cfe-commits


@@ -214,7 +232,8 @@ declare double @za_shared_callee(double) "aarch64_inout_za"
 define double  @za_new_caller_to_za_shared_callee(double %x) nounwind noinline 
optnone "aarch64_new_za"{
 ; CHECK-COMMON-LABEL: za_new_caller_to_za_shared_callee:
 ; CHECK-COMMON:   // %bb.0: // %prelude
-; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-16]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-32]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:str x19, [sp, #16] // 8-byte Folded Spill

efriedma-quic wrote:

Oh, right, it's not directly connected to this patch, it's just because you're 
changing the RUN line.

Please file a bug for this.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-10 Thread Kerry McLaughlin via cfe-commits


@@ -214,7 +232,8 @@ declare double @za_shared_callee(double) "aarch64_inout_za"
 define double  @za_new_caller_to_za_shared_callee(double %x) nounwind noinline 
optnone "aarch64_new_za"{
 ; CHECK-COMMON-LABEL: za_new_caller_to_za_shared_callee:
 ; CHECK-COMMON:   // %bb.0: // %prelude
-; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-16]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-32]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:str x19, [sp, #16] // 8-byte Folded Spill

kmclaughlin-arm wrote:

The base pointer x19 is added to the list of saved registers in 
`determineCalleeSaves` if the function is in streaming mode or has SVE. The 
spill was introduced here when I enabled SVE for this test.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-10 Thread Kerry McLaughlin via cfe-commits


@@ -196,12 +196,14 @@ bool AArch64FunctionInfo::needsAsyncDwarfUnwindInfo(
 const MachineFunction &MF) const {
   if (!NeedsAsyncDwarfUnwindInfo) {
 const Function &F = MF.getFunction();
+const AArch64FunctionInfo *AFI = MF.getInfo();
 //  The check got "minsize" is because epilogue unwind info is not emitted
 //  (yet) for homogeneous epilogues, outlined functions, and functions
 //  outlined from.
-NeedsAsyncDwarfUnwindInfo = needsDwarfUnwindInfo(MF) &&
-F.getUWTableKind() == UWTableKind::Async &&
-!F.hasMinSize();
+NeedsAsyncDwarfUnwindInfo =
+(needsDwarfUnwindInfo(MF) && F.getUWTableKind() == UWTableKind::Async 
&&

kmclaughlin-arm wrote:

This PR does include a change to 
`AArch64FrameLowering::homogeneousPrologEpilog` which disables homogeneous 
epilogues if the function has streaming-mode changes.

I hadn't considered outlining, but I can see that when considering candidates 
we must be able to outline all CFI instructions in the function. Am I correct 
in thinking that this is the reason we would need to disable outlining when 
there are streaming-mode changes which require async unwind info?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-07 Thread Eli Friedman via cfe-commits


@@ -196,12 +196,14 @@ bool AArch64FunctionInfo::needsAsyncDwarfUnwindInfo(
 const MachineFunction &MF) const {
   if (!NeedsAsyncDwarfUnwindInfo) {
 const Function &F = MF.getFunction();
+const AArch64FunctionInfo *AFI = MF.getInfo();
 //  The check got "minsize" is because epilogue unwind info is not emitted
 //  (yet) for homogeneous epilogues, outlined functions, and functions
 //  outlined from.
-NeedsAsyncDwarfUnwindInfo = needsDwarfUnwindInfo(MF) &&
-F.getUWTableKind() == UWTableKind::Async &&
-!F.hasMinSize();
+NeedsAsyncDwarfUnwindInfo =
+(needsDwarfUnwindInfo(MF) && F.getUWTableKind() == UWTableKind::Async 
&&

efriedma-quic wrote:

Oh, also, this should probably be `NeedsAsyncDwarfUnwindInfo = 
needsDwarfUnwindInfo(MF) && ((F.getUWTableKind() == UWTableKind::Async && 
!F.hasMinSize()) || AFI->hasStreamingModeChanges());`.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-07 Thread Eli Friedman via cfe-commits


@@ -214,7 +232,8 @@ declare double @za_shared_callee(double) "aarch64_inout_za"
 define double  @za_new_caller_to_za_shared_callee(double %x) nounwind noinline 
optnone "aarch64_new_za"{
 ; CHECK-COMMON-LABEL: za_new_caller_to_za_shared_callee:
 ; CHECK-COMMON:   // %bb.0: // %prelude
-; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-16]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:stp x29, x30, [sp, #-32]! // 16-byte Folded Spill
+; CHECK-COMMON-NEXT:str x19, [sp, #16] // 8-byte Folded Spill

efriedma-quic wrote:

I feel like I must have asked about this at some point, but where is the x19 
spill coming from?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-07 Thread Eli Friedman via cfe-commits


@@ -196,12 +196,14 @@ bool AArch64FunctionInfo::needsAsyncDwarfUnwindInfo(
 const MachineFunction &MF) const {
   if (!NeedsAsyncDwarfUnwindInfo) {
 const Function &F = MF.getFunction();
+const AArch64FunctionInfo *AFI = MF.getInfo();
 //  The check got "minsize" is because epilogue unwind info is not emitted
 //  (yet) for homogeneous epilogues, outlined functions, and functions
 //  outlined from.
-NeedsAsyncDwarfUnwindInfo = needsDwarfUnwindInfo(MF) &&
-F.getUWTableKind() == UWTableKind::Async &&
-!F.hasMinSize();
+NeedsAsyncDwarfUnwindInfo =
+(needsDwarfUnwindInfo(MF) && F.getUWTableKind() == UWTableKind::Async 
&&

efriedma-quic wrote:

If we can't emit correct async unwind info for functions with 
outlining/homogeneous epilogues, does that mean we also need to disable 
outlining/homogeneous epilogues for functions with streaming mode changes?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-07 Thread Kerry McLaughlin via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

kmclaughlin-arm wrote:

I've made changes to `needsAsyncDwarfUnwindInfo` in 
AArch64MachineFunctionInfo.cpp to always return true if the function has 
streaming-mode changes. I believe this will ensure we emit the correct 
information when `-fno-asynchronous-unwind-tables` is specified.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-05 Thread Eli Friedman via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

efriedma-quic wrote:

Oh, right, there's also that dimension.  I'm not sure I understand the 
interaction here, but if there's an issue, can we just force on "asynchronous" 
unwind info in that case?  The point of non-async unwind info isn't that it's a 
different unwind format; it's just an optimization to reduce the size of the 
unwind info.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-05 Thread Sander de Smalen via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

sdesmalen-arm wrote:

@efriedma-quic I think the issue is that when we emit unwind info that is _not_ 
asynchronous, then the unwinder can't correctly unwind the stack because it 
would use the wrong value for VG to compute the offsets of callee-saves. So any 
unwind info that would be produced is broken.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-04 Thread Eli Friedman via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

efriedma-quic wrote:

If there's no DWARF unwind, nothing can unwind the stack whether or not we 
store the VG.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-04 Thread Kerry McLaughlin via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

kmclaughlin-arm wrote:

If we still emit the spill of VG with `-fno-asynchronous-unwind-tables`, I 
don't know how we would be able to recover the correct value without the 
per-call CFI saves and restores. And without the correct VG value, I don't 
think it will be possible to recover any VG based values in the stack frame.

I'm still not sure why this wouldn't require a diagnostic, because if there is 
not enough information to recover VG then I don't think we can unwind correctly?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-03 Thread Eli Friedman via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

efriedma-quic wrote:

There are two different kinds of DWARF "unwind info"; one is the kind that's in 
a loadable section, and used for EH. The other is in a debug info section, not 
loaded at runtime.  If you specify `-fasynchronous-unwind-tables`, you get the 
former; if you specify `-g -fno-asynchronous-unwind-tables`, you get the latter.

If you request no debug info and no unwind tables, we shouldn't emit any DWARF 
directives.

But like I mentioned, I think we want to unconditionally emit the code to save 
the VG, whether or not we emit the corresponding DWARF directives.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-03 Thread Kerry McLaughlin via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

kmclaughlin-arm wrote:

> when I compile some code with -fno-asynchronous-unwind-tables, it will still 
> generate these directives. Perhaps this should have a diagnostic in Clang?

I'm happy to add such a diagnostic, but I want to make sure I have the reason 
for this requirement correct.
If I understand correctly, enabling asychronous unwind tables is required now 
that I am emitting the save and restore of VG at the point of each call in the 
function which changes streaming-mode.

> we want debuggers to be able to unwind the stack even if a function is 
> nounwind.

Given this, I'm not entirely sure why we would expect to be able to unwind if 
`-fno-asynchronous-unwind-tables` was used or if the function has been marked 
as nounwind? I might have misunderstood something though!

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-03 Thread Kerry McLaughlin via cfe-commits


@@ -3768,6 +3768,12 @@ def err_conflicting_attributes_arm_state : Error<
   "conflicting attributes for state '%0'">;
 def err_sme_streaming_cannot_be_multiversioned : Error<
   "streaming function cannot be multi-versioned">;
+def err_sme_streaming_mode_change_no_sve : Error<
+  "function requires a streaming-mode change, unwinding is not possible 
without 'sve'. "
+  "Consider marking this function as 'noexcept' or 
'__attribute__((nothrow))'">;

kmclaughlin-arm wrote:

Documentation for this function was added in 
https://github.com/ARM-software/abi-aa/pull/263 and 
https://github.com/llvm/llvm-project/pull/92921 adds the routine to compiler-rt.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-03 Thread Kerry McLaughlin via cfe-commits


@@ -1552,6 +1553,57 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock 
&MBB,
case AArch64::COALESCER_BARRIER_FPR128:
  MI.eraseFromParent();
  return true;
+   case AArch64::VGSavePseudo:
+   case AArch64::VGRestorePseudo: {
+ MachineFunction &MF = *MBB.getParent();
+ SMEAttrs FuncAttrs(MF.getFunction());
+ bool LocallyStreaming =
+ FuncAttrs.hasStreamingBody() && !FuncAttrs.hasStreamingInterface();
+ const AArch64FunctionInfo *AFI = MF.getInfo();
+
+ if (!AFI->requiresVGSpill(MF))
+   return false;
+
+ int64_t VGFrameIdx =
+ LocallyStreaming ? AFI->getStreamingVGIdx() : AFI->getVGIdx();
+ assert(VGFrameIdx != std::numeric_limits::max() &&
+"Expected FrameIdx for VG");
+
+ const TargetSubtargetInfo &STI = MF.getSubtarget();
+ const TargetInstrInfo &TII = *STI.getInstrInfo();
+ const TargetRegisterInfo &TRI = *STI.getRegisterInfo();
+
+ if (Opcode == AArch64::VGSavePseudo) {
+   // This pseudo has been inserted after a streaming-mode change
+   // to save the streaming value of VG before a call.
+   // Calculate and emit the CFI offset using VGFrameIdx.
+   MachineFrameInfo &MFI = MF.getFrameInfo();
+   const AArch64FrameLowering *TFI =
+   MF.getSubtarget().getFrameLowering();
+
+   int64_t Offset =
+   MFI.getObjectOffset(VGFrameIdx) - TFI->getOffsetOfLocalArea();
+   unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
+   nullptr, TRI.getDwarfRegNum(AArch64::VG, true), Offset));
+   BuildMI(MBB, MBBI, MBBI->getDebugLoc(),
+   TII.get(TargetOpcode::CFI_INSTRUCTION))
+   .addCFIIndex(CFIIndex)
+   .setMIFlags(MachineInstr::FrameSetup);
+ } else {

kmclaughlin-arm wrote:

Expanding the pseudos has moved to AArch64FrameLowering.cpp. There is more 
common code here, so I have only added one function to expand both 
(`emitVGSaveRestore`). I can split this into an `emitVGSave`/`emitVGRestore` if 
you'd still prefer they be kept separate.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-06-03 Thread Kerry McLaughlin via cfe-commits


@@ -1552,6 +1553,57 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock 
&MBB,
case AArch64::COALESCER_BARRIER_FPR128:
  MI.eraseFromParent();
  return true;
+   case AArch64::VGSavePseudo:
+   case AArch64::VGRestorePseudo: {
+ MachineFunction &MF = *MBB.getParent();
+ SMEAttrs FuncAttrs(MF.getFunction());
+ bool LocallyStreaming =
+ FuncAttrs.hasStreamingBody() && !FuncAttrs.hasStreamingInterface();
+ const AArch64FunctionInfo *AFI = MF.getInfo();
+
+ if (!AFI->requiresVGSpill(MF))
+   return false;
+
+ int64_t VGFrameIdx =
+ LocallyStreaming ? AFI->getStreamingVGIdx() : AFI->getVGIdx();
+ assert(VGFrameIdx != std::numeric_limits::max() &&
+"Expected FrameIdx for VG");
+
+ const TargetSubtargetInfo &STI = MF.getSubtarget();
+ const TargetInstrInfo &TII = *STI.getInstrInfo();
+ const TargetRegisterInfo &TRI = *STI.getRegisterInfo();
+
+ if (Opcode == AArch64::VGSavePseudo) {
+   // This pseudo has been inserted after a streaming-mode change
+   // to save the streaming value of VG before a call.
+   // Calculate and emit the CFI offset using VGFrameIdx.
+   MachineFrameInfo &MFI = MF.getFrameInfo();
+   const AArch64FrameLowering *TFI =
+   MF.getSubtarget().getFrameLowering();
+
+   int64_t Offset =
+   MFI.getObjectOffset(VGFrameIdx) - TFI->getOffsetOfLocalArea();
+   unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
+   nullptr, TRI.getDwarfRegNum(AArch64::VG, true), Offset));
+   BuildMI(MBB, MBBI, MBBI->getDebugLoc(),
+   TII.get(TargetOpcode::CFI_INSTRUCTION))
+   .addCFIIndex(CFIIndex)
+   .setMIFlags(MachineInstr::FrameSetup);
+ } else {
+   // This is a restore of VG after returning from the call. Emit the
+   // .cfi_restore instruction, which sets the rule for VG to the same
+   // as it was on entry to the function.
+   ++MBBI;

kmclaughlin-arm wrote:

This was a mistake, the iterator shouldn't be changing here as the pseudos are 
emitted in the correct place around the call.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Eli Friedman via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

efriedma-quic wrote:

I'm not sure I follow what the issue is... I think we discussed before that we 
want debuggers to be able to unwind the stack even if a function is nounwind.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Eli Friedman via cfe-commits


@@ -3768,6 +3768,12 @@ def err_conflicting_attributes_arm_state : Error<
   "conflicting attributes for state '%0'">;
 def err_sme_streaming_cannot_be_multiversioned : Error<
   "streaming function cannot be multi-versioned">;
+def err_sme_streaming_mode_change_no_sve : Error<
+  "function requires a streaming-mode change, unwinding is not possible 
without 'sve'. "
+  "Consider marking this function as 'noexcept' or 
'__attribute__((nothrow))'">;

efriedma-quic wrote:

If we call new functions, they need to be part of the ABI.  If you're happy to 
work with your ABI people to document the new interface, I guess it's not a 
problem.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -1552,6 +1553,57 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock 
&MBB,
case AArch64::COALESCER_BARRIER_FPR128:
  MI.eraseFromParent();
  return true;
+   case AArch64::VGSavePseudo:
+   case AArch64::VGRestorePseudo: {
+ MachineFunction &MF = *MBB.getParent();
+ SMEAttrs FuncAttrs(MF.getFunction());
+ bool LocallyStreaming =
+ FuncAttrs.hasStreamingBody() && !FuncAttrs.hasStreamingInterface();
+ const AArch64FunctionInfo *AFI = MF.getInfo();
+
+ if (!AFI->requiresVGSpill(MF))
+   return false;
+
+ int64_t VGFrameIdx =
+ LocallyStreaming ? AFI->getStreamingVGIdx() : AFI->getVGIdx();
+ assert(VGFrameIdx != std::numeric_limits::max() &&
+"Expected FrameIdx for VG");
+
+ const TargetSubtargetInfo &STI = MF.getSubtarget();
+ const TargetInstrInfo &TII = *STI.getInstrInfo();
+ const TargetRegisterInfo &TRI = *STI.getRegisterInfo();
+
+ if (Opcode == AArch64::VGSavePseudo) {
+   // This pseudo has been inserted after a streaming-mode change
+   // to save the streaming value of VG before a call.
+   // Calculate and emit the CFI offset using VGFrameIdx.
+   MachineFrameInfo &MFI = MF.getFrameInfo();
+   const AArch64FrameLowering *TFI =
+   MF.getSubtarget().getFrameLowering();
+
+   int64_t Offset =
+   MFI.getObjectOffset(VGFrameIdx) - TFI->getOffsetOfLocalArea();
+   unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
+   nullptr, TRI.getDwarfRegNum(AArch64::VG, true), Offset));
+   BuildMI(MBB, MBBI, MBBI->getDebugLoc(),
+   TII.get(TargetOpcode::CFI_INSTRUCTION))
+   .addCFIIndex(CFIIndex)
+   .setMIFlags(MachineInstr::FrameSetup);
+ } else {
+   // This is a restore of VG after returning from the call. Emit the
+   // .cfi_restore instruction, which sets the rule for VG to the same
+   // as it was on entry to the function.
+   ++MBBI;

sdesmalen-arm wrote:

I'm not sure why you're incrementing the iterator? I don't think this makes a 
difference if you remove the pseudo? In fact, when I remove this all the tests 
still pass.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -223,6 +228,12 @@ class AArch64FunctionInfo final : public 
MachineFunctionInfo {
   Register getPStateSMReg() const { return PStateSMReg; };
   void setPStateSMReg(Register Reg) { PStateSMReg = Reg; };
 
+  int64_t getVGIdx() const { return VGIdx; };
+  void setVGIdx(unsigned Idx) { VGIdx = Idx; };
+
+  int64_t getStreamingVGIdx() const { return StreamingVGIdx; };
+  void setStreamingVGIdx(unsigned Idx) { StreamingVGIdx = Idx; };

sdesmalen-arm wrote:

minor nit: Could you use `FrameIdx` instead of just `Idx`?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -8443,9 +8452,16 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   if (RequiresSMChange) {
 assert(PStateSM && "Expected a PStateSM to be set");
+

sdesmalen-arm wrote:

nit: unnecessary newline.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -1552,6 +1553,57 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock 
&MBB,
case AArch64::COALESCER_BARRIER_FPR128:
  MI.eraseFromParent();
  return true;
+   case AArch64::VGSavePseudo:
+   case AArch64::VGRestorePseudo: {
+ MachineFunction &MF = *MBB.getParent();
+ SMEAttrs FuncAttrs(MF.getFunction());
+ bool LocallyStreaming =
+ FuncAttrs.hasStreamingBody() && !FuncAttrs.hasStreamingInterface();
+ const AArch64FunctionInfo *AFI = MF.getInfo();
+
+ if (!AFI->requiresVGSpill(MF))
+   return false;
+
+ int64_t VGFrameIdx =
+ LocallyStreaming ? AFI->getStreamingVGIdx() : AFI->getVGIdx();
+ assert(VGFrameIdx != std::numeric_limits::max() &&
+"Expected FrameIdx for VG");
+
+ const TargetSubtargetInfo &STI = MF.getSubtarget();
+ const TargetInstrInfo &TII = *STI.getInstrInfo();

sdesmalen-arm wrote:

nit: `TII` is already available as a member of `AArch64ExpandPseudo`, no need 
to get it again.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -1552,6 +1553,57 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock 
&MBB,
case AArch64::COALESCER_BARRIER_FPR128:
  MI.eraseFromParent();
  return true;
+   case AArch64::VGSavePseudo:
+   case AArch64::VGRestorePseudo: {
+ MachineFunction &MF = *MBB.getParent();
+ SMEAttrs FuncAttrs(MF.getFunction());
+ bool LocallyStreaming =
+ FuncAttrs.hasStreamingBody() && !FuncAttrs.hasStreamingInterface();
+ const AArch64FunctionInfo *AFI = MF.getInfo();
+
+ if (!AFI->requiresVGSpill(MF))
+   return false;
+
+ int64_t VGFrameIdx =
+ LocallyStreaming ? AFI->getStreamingVGIdx() : AFI->getVGIdx();

sdesmalen-arm wrote:

It would be nice if we could keep all this knowledge within the FrameLowering 
without having information that we implicitly pass (through AFI) between 
different passes (in this case, PEI and PseudoExpansion).

PEI has a callback named `processFunctionBeforeFrameIndicesReplaced`. You could 
update the `VGSave/RestorePseudo` nodes to add the offset, so that the code 
here simply has to replace the pseudo by a CFI_INSTRUCTION. Or you could 
replace the pseudo in that callback itself.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -8287,6 +8289,13 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   SDValue InGlue;
   if (RequiresSMChange) {
+
+if (Subtarget->hasSVE()) {

sdesmalen-arm wrote:

I think we can only emit this pseudo when we have asynchronous unwind tables 
enabled. At the moment, when I compile some code with 
`-fno-asynchronous-unwind-tables`, it will still generate these directives. 
Perhaps this should have a diagnostic in Clang?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -1552,6 +1553,57 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock 
&MBB,
case AArch64::COALESCER_BARRIER_FPR128:
  MI.eraseFromParent();
  return true;
+   case AArch64::VGSavePseudo:
+   case AArch64::VGRestorePseudo: {
+ MachineFunction &MF = *MBB.getParent();
+ SMEAttrs FuncAttrs(MF.getFunction());
+ bool LocallyStreaming =
+ FuncAttrs.hasStreamingBody() && !FuncAttrs.hasStreamingInterface();
+ const AArch64FunctionInfo *AFI = MF.getInfo();
+
+ if (!AFI->requiresVGSpill(MF))
+   return false;

sdesmalen-arm wrote:

This can be removed, because the pseudo should not have been emitted if the 
function didn't require a spill of VG.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -1552,6 +1553,57 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock 
&MBB,
case AArch64::COALESCER_BARRIER_FPR128:
  MI.eraseFromParent();
  return true;
+   case AArch64::VGSavePseudo:
+   case AArch64::VGRestorePseudo: {
+ MachineFunction &MF = *MBB.getParent();
+ SMEAttrs FuncAttrs(MF.getFunction());
+ bool LocallyStreaming =
+ FuncAttrs.hasStreamingBody() && !FuncAttrs.hasStreamingInterface();
+ const AArch64FunctionInfo *AFI = MF.getInfo();
+
+ if (!AFI->requiresVGSpill(MF))
+   return false;
+
+ int64_t VGFrameIdx =
+ LocallyStreaming ? AFI->getStreamingVGIdx() : AFI->getVGIdx();
+ assert(VGFrameIdx != std::numeric_limits::max() &&
+"Expected FrameIdx for VG");
+
+ const TargetSubtargetInfo &STI = MF.getSubtarget();
+ const TargetInstrInfo &TII = *STI.getInstrInfo();
+ const TargetRegisterInfo &TRI = *STI.getRegisterInfo();
+
+ if (Opcode == AArch64::VGSavePseudo) {
+   // This pseudo has been inserted after a streaming-mode change
+   // to save the streaming value of VG before a call.
+   // Calculate and emit the CFI offset using VGFrameIdx.
+   MachineFrameInfo &MFI = MF.getFrameInfo();
+   const AArch64FrameLowering *TFI =
+   MF.getSubtarget().getFrameLowering();
+
+   int64_t Offset =
+   MFI.getObjectOffset(VGFrameIdx) - TFI->getOffsetOfLocalArea();
+   unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
+   nullptr, TRI.getDwarfRegNum(AArch64::VG, true), Offset));
+   BuildMI(MBB, MBBI, MBBI->getDebugLoc(),
+   TII.get(TargetOpcode::CFI_INSTRUCTION))
+   .addCFIIndex(CFIIndex)
+   .setMIFlags(MachineInstr::FrameSetup);
+ } else {

sdesmalen-arm wrote:

There is little common code between the two, so I'd rather see this written as:
```
case AArch64::VGSavePseudo: {
  ...
}
case AArch64::VGRestorePseudo: {
  ...
}
```

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-15 Thread Sander de Smalen via cfe-commits


@@ -3768,6 +3768,12 @@ def err_conflicting_attributes_arm_state : Error<
   "conflicting attributes for state '%0'">;
 def err_sme_streaming_cannot_be_multiversioned : Error<
   "streaming function cannot be multi-versioned">;
+def err_sme_streaming_mode_change_no_sve : Error<
+  "function requires a streaming-mode change, unwinding is not possible 
without 'sve'. "
+  "Consider marking this function as 'noexcept' or 
'__attribute__((nothrow))'">;

sdesmalen-arm wrote:

It probably makes more sense to add a routine to compiler-rt that returns the 
value of VG if SVE is available, rather than emitting an error here. You can 
implement that function using the (already existing) interfaces for 
function-multiversioning to check if SVE is available at runtime. In that case, 
I think there is little value in having these Clang changes here.

@efriedma-quic are you happy going with that approach instead?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-09 Thread Eli Friedman via cfe-commits

https://github.com/efriedma-quic approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-09 Thread Kerry McLaughlin via cfe-commits


@@ -221,6 +224,15 @@ def : Pat<(AArch64_smstop (i32 svcr_op:$pstate), (i64 
/*AArch64SME::Always*/0)),
   (MSRpstatesvcrImm1 svcr_op:$pstate, 0b0)>;
 
 
+// Pseudo to insert cfi_offset/cfi_restore instructions. Used to save or 
restore
+// the streaming value of VG around streaming-mode changes in locally-streaming
+// functions.
+def VGUnwindInfoPseudo :
+  Pseudo<(outs), (ins timm0_1:$save_restore), []>, Sched<[]>;

kmclaughlin-arm wrote:

There wasn't any particular reason for only adding only pseudo, so I've split 
this out into two (VGSavePseudo & VGRestorePseudo).

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-09 Thread Kerry McLaughlin via cfe-commits


@@ -3730,6 +3730,12 @@ def warn_gnu_inline_cplusplus_without_extern : Warning<
   "'gnu_inline' attribute without 'extern' in C++ treated as externally"
   " available, this changed in Clang 10">,
   InGroup>;
+def warn_sme_streaming_mode_change_no_sve : Warning<
+  "function requires a streaming-mode change, unwinding is not possible 
without 'sve'">,
+  InGroup;

kmclaughlin-arm wrote:

Thanks @efriedma-quic, I have changed these warnings to errors as suggested and 
also updated SemaChecking.cpp to only emit them if the callee is not marked 
with either `noexcept` or `nothrow`. I thought that we should not emit the 
errors if `fno-exceptions` is used as well, so I've added a check for 
`getLangOpts().Exceptions` in the same place.

I started adding similar asserts to LLVM to ensure that if SVE is not 
available, that nounwind is set on the callee. I haven't included these in the 
latest commit however, as many of the LLVM tests for SME do not currently pass 
`-mattr=+sve` and would require updating a lot of CHECK lines. I would prefer 
to work on this separately and post a new patch following this PR if possible, 
as I think including all of the test changes will make this PR more difficult 
to review.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-05 Thread Eli Friedman via cfe-commits


@@ -221,6 +224,15 @@ def : Pat<(AArch64_smstop (i32 svcr_op:$pstate), (i64 
/*AArch64SME::Always*/0)),
   (MSRpstatesvcrImm1 svcr_op:$pstate, 0b0)>;
 
 
+// Pseudo to insert cfi_offset/cfi_restore instructions. Used to save or 
restore
+// the streaming value of VG around streaming-mode changes in locally-streaming
+// functions.
+def VGUnwindInfoPseudo :
+  Pseudo<(outs), (ins timm0_1:$save_restore), []>, Sched<[]>;

efriedma-quic wrote:

Is there a reason to make this one pseudo, instead of two?  The two operations 
have opposite semantics, and opcode space isn't that scarce.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-05-05 Thread Eli Friedman via cfe-commits


@@ -3730,6 +3730,12 @@ def warn_gnu_inline_cplusplus_without_extern : Warning<
   "'gnu_inline' attribute without 'extern' in C++ treated as externally"
   " available, this changed in Clang 10">,
   InGroup>;
+def warn_sme_streaming_mode_change_no_sve : Warning<
+  "function requires a streaming-mode change, unwinding is not possible 
without 'sve'">,
+  InGroup;

efriedma-quic wrote:

This should probably be an error if it's possible to unwind: it's effectively 
miscompile.  Both here, and in the backend.  Add a note suggesting marking the 
function `noexcept`/`__attribute((nothrow))`.

If it isn't possible to unwind, you just end up with slightly inaccurate debug 
info, which is just annoying; probably not worth warning for that.

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-04-30 Thread Kerry McLaughlin via cfe-commits

kmclaughlin-arm wrote:

Gentle ping :)

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-04-22 Thread Kerry McLaughlin via cfe-commits


@@ -8001,6 +8007,22 @@ void Sema::checkCall(NamedDecl *FDecl, const 
FunctionProtoType *Proto,
   }
 }
 
+// SME functions may require SVE to be available for unwinding, as the
+// value of VG needs to be preserved across streaming-mode changes.
+if (CallerFD && (!FD || !FD->getBuiltinID()) &&
+!Context.getTargetInfo().hasFeature("sve")) {
+  if (CallerFD->hasAttr())
+Diag(Loc, diag::warn_sme_locally_streaming_no_sve);
+
+auto CallerStreamingTy = getArmStreamingFnType(CallerFD);

kmclaughlin-arm wrote:

Fixed in latest commit

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)

2024-04-19 Thread Eli Friedman via cfe-commits


@@ -8001,6 +8007,22 @@ void Sema::checkCall(NamedDecl *FDecl, const 
FunctionProtoType *Proto,
   }
 }
 
+// SME functions may require SVE to be available for unwinding, as the
+// value of VG needs to be preserved across streaming-mode changes.
+if (CallerFD && (!FD || !FD->getBuiltinID()) &&
+!Context.getTargetInfo().hasFeature("sve")) {
+  if (CallerFD->hasAttr())
+Diag(Loc, diag::warn_sme_locally_streaming_no_sve);
+
+auto CallerStreamingTy = getArmStreamingFnType(CallerFD);

efriedma-quic wrote:

Indentation?

https://github.com/llvm/llvm-project/pull/83301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits