Page MenuHomePhabricator

clang/AMDGPU: Fix default for frame-pointer attribute
ClosedPublic

Authored by arsenm on Nov 19 2019, 2:02 AM.

Details

Summary

Enabling optimization should allow frame pointer elimination.

Diff Detail

Event Timeline

arsenm created this revision.Nov 19 2019, 2:02 AM

LGTM. But I am wondering how it affects -g. Do we need to keep frame pointer when -g is specified? Should we add a test for -O3 -g?

t-tye added a comment.Nov 25 2019, 1:01 PM

@scott.linder can answer about the -g question, but I would expect that the CFI is capable of describing the address of the CFA regardless of whether there is a frame pointer by simply knowing the constant offset from the stack pointer.

For AMDGPU it seems to me what we really have is an FP and we optimize away the SP since the stack grows low address to high address, and S32 points to the base of the frame, and not the top of the stack.

@scott.linder can answer about the -g question, but I would expect that the CFI is capable of describing the address of the CFA regardless of whether there is a frame pointer by simply knowing the constant offset from the stack pointer.

For AMDGPU it seems to me what we really have is an FP and we optimize away the SP since the stack grows low address to high address, and S32 points to the base of the frame, and not the top of the stack.

Right, with CFI we will be able to describe every case we can generate code for, regardless of whether we optimize out the FP.

LGTM. But I am wondering how it affects -g. Do we need to keep frame pointer when -g is specified? Should we add a test for -O3 -g?

It’s independent for every other target. This matches the standard behavior

LGTM. But I am wondering how it affects -g. Do we need to keep frame pointer when -g is specified? Should we add a test for -O3 -g?

It’s independent for every other target. This matches the standard behavior

Also -g should not change codegen, but FP elimination would obviously be a change

This revision is now accepted and ready to land.Dec 6 2019, 10:40 AM
arsenm closed this revision.Dec 6 2019, 10:40 AM