Skip to content

[AArch64][SVE] Move incorrectly placed assert #144318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 17, 2025
Merged

Conversation

MacDue
Copy link
Member

@MacDue MacDue commented Jun 16, 2025

This assert is only valid if FPAfterSVECalleeSaves is true, for the default layout resolving CSR works correctly.

This assert is only valid if FPAfterSVECalleeSaves is true, for the
default layout resolving CSR works correctly.
@llvmbot
Copy link
Member

llvmbot commented Jun 16, 2025

@llvm/pr-subscribers-backend-aarch64

Author: Benjamin Maxwell (MacDue)

Changes

This assert is only valid if FPAfterSVECalleeSaves is true, for the default layout resolving CSR works correctly.


Full diff: https://github.com/llvm/llvm-project/pull/144318.diff

1 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64FrameLowering.cpp (+2-2)
diff --git a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
index 2650c621e19f6..787a74b978999 100644
--- a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
@@ -2896,8 +2896,6 @@ StackOffset AArch64FrameLowering::resolveFrameOffsetReference(
       isTargetWindows(MF) && AFI->getSVECalleeSavedStackSize();
 
   if (isSVE) {
-    assert(-ObjectOffset > (int64_t)AFI->getSVECalleeSavedStackSize() &&
-           "Math isn't correct for CSRs with FPAfterSVECalleeSaves");
     StackOffset FPOffset =
         StackOffset::get(-AFI->getCalleeSaveBaseToFrameRecordOffset(), ObjectOffset);
     StackOffset SPOffset =
@@ -2905,6 +2903,8 @@ StackOffset AArch64FrameLowering::resolveFrameOffsetReference(
         StackOffset::get(MFI.getStackSize() - AFI->getCalleeSavedStackSize(),
                          ObjectOffset);
     if (FPAfterSVECalleeSaves) {
+      assert(-ObjectOffset > (int64_t)AFI->getSVECalleeSavedStackSize() &&
+           "Math isn't correct for CSRs with FPAfterSVECalleeSaves");
       FPOffset += StackOffset::getScalable(AFI->getSVECalleeSavedStackSize());
     }
     // Always use the FP for SVE spills if available and beneficial.

Copy link

github-actions bot commented Jun 16, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I was sort of hoping it would hold for both... now I'll need to track down what's calling it this way, write a testcase, and actually implement it. (I think the calls are coming from debug info generation, but I'm not sure why debug info is making this query.)

@MacDue MacDue merged commit 437945b into llvm:main Jun 17, 2025
7 checks passed
@MacDue MacDue deleted the move_assert branch June 17, 2025 08:26
ajaden-codes pushed a commit to Jaddyen/llvm-project that referenced this pull request Jun 17, 2025
This assert is only valid if FPAfterSVECalleeSaves is true, for the
default layout resolving CSR works correctly.
fschlimb pushed a commit to fschlimb/llvm-project that referenced this pull request Jun 18, 2025
This assert is only valid if FPAfterSVECalleeSaves is true, for the
default layout resolving CSR works correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants