Skip to content
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

[PEI][PowerPC] Fix false alarm of stack size limit #65559

Merged
merged 10 commits into from
Sep 8, 2023
Merged

Conversation

bzEq
Copy link
Collaborator

@bzEq bzEq commented Sep 7, 2023

PPC64 allows stack size up to ((2^63)-1) bytes. Currently llc reports

warning: stack frame size (4294967568) exceeds limit (4294967295) in function 'main'

if the stack allocated is larger than 4G.

; CHECK-NOT: warning: {{.*}} stack frame size ({{.*}}) exceeds limit (4294967295) in function 'large_stack'
define i8* @large_stack() {
%s = alloca [281474976710656 x i8], align 1
%e = getelementptr i8, i8* %s, i64 0
Copy link
Member

Choose a reason for hiding this comment

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

use opaque pointers ptr

Does it really work for a larger-than-4G limit? Do you not hit any other limit in LLVM codegen that might assume uint32_t or a smaller limit? You probably want to try more more stack operations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On PPC64, larger-than-4G stack was supported after 5018a5d.

Copy link
Collaborator Author

@bzEq bzEq Sep 7, 2023

Choose a reason for hiding this comment

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

And we have some internal users using stack larger than 4G.

@bzEq bzEq requested a review from MaskRay September 7, 2023 05:23
Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

LGTM; thanks! 🚋 ✅

@@ -2740,3 +2740,11 @@ bool PPCFrameLowering::enableShrinkWrapping(const MachineFunction &MF) const {
return false;
return !MF.getSubtarget<PPCSubtarget>().is32BitELFABI();
}

uint64_t PPCFrameLowering::getStackThreshold() const {
// The scratch register of STUX contains a signed negative 64-bit number.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very familiar with ppc, but this comment doesn't mean much to me. Maybe other reviewers with more familiarity can triple check it?

Copy link
Member

@nemanjai nemanjai left a comment

Choose a reason for hiding this comment

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

Were we always getting this warning on the tests added in 5018a5d?
My initial reaction to this was that the test case needs to show an actual access on a large stack frame, but we already have that in test/CodeGen/PowerPC/huge-frame-size.ll
Maybe it would be nice to add these cases to the same file since this is very related.

Otherwise, LGTM

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp Outdated Show resolved Hide resolved
@bzEq
Copy link
Collaborator Author

bzEq commented Sep 8, 2023

Were we always getting this warning on the tests added in 5018a5d?

I think so, I didn't catch that in FileCheck. For current llc run on huge-frame-size.ll.

warning: <unknown>:0:0: stack frame size (4294967344) exceeds limit (4294967295) in function 'foo'

@bzEq bzEq merged commit d9efcb5 into llvm:main Sep 8, 2023
2 checks passed
@bzEq bzEq deleted the stack-threshold branch September 8, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants