[clang] [llvm] [AArch64][SME] Save VG for unwind info when changing streaming-mode (PR #83301)
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)
@@ -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)
@@ -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)
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)
@@ -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)
@@ -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)
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)
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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
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)
@@ -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)
@@ -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
