Page MenuHomePhabricator

[AVR] Remove redundant dynalloca SP save/restore pass
ClosedPublic

Authored by aykevl on Mar 2 2021, 4:00 PM.

Details

Summary

I think this pass was previously used under the assumption that most
functions would not need a frame pointer and it would be more efficient
to store the old stack pointer in a regular register pair.

Unfortunately, right now we're forced to always reserve the Y register
as a frame pointer: whether or not this is needed is only known after
regsiter allocation at which point it doesn't make sense anymore to mark
it as non-reserved. Therefore, it makes sense to use the Y register to
store the old stack pointer in functions with dynamic allocas (with a
variable size or not in the entry block). Knowing this can make the code
around dynamic allocas a lot simpler: simply save/restore the frame
pointer.

This is especially relevant in functions that have a frame pointer
anyway (for example, because they have stack spills). The stack restore
in the epilogue will implicitly restore the old stack pointer, so there
is no need to store the old stack pointer separately. It even reduces
register pressure as a side effect.


Warning: I haven't really tested this outside of the unit tests (and studying the output assembly). I'm not exactly sure how to test this, maybe by linking some test code with an existing program to check that it works as expected? However, the assembly looks good to me.
I checked compiler-rt and picolibc and both don't have any dynamic stack allocation (as I should have expected).

To be clear: this patch is meant as a refactor/cleanup, not as a codegen improvement (even though it is a codegen improvement).

Diff Detail

Event Timeline

aykevl created this revision.Mar 2 2021, 4:00 PM
aykevl requested review of this revision.Mar 2 2021, 4:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2021, 4:00 PM
aykevl edited the summary of this revision. (Show Details)Mar 3 2021, 4:58 AM

It looks like this is related to the following bug: https://bugs.llvm.org/show_bug.cgi?id=31349

aykevl added a comment.Jan 4 2022, 3:01 PM

Ping?
I'm working on an optimization that appears to require this patch.

benshi001 added a comment.EditedJan 8 2022, 6:08 AM

So your patch only affects functions which has variable size objects,

  1. Use frame pointer
  2. Use the fixed frame pointer (Y register) instead of dynamically allocated register pair to store the old SP.
benshi001 accepted this revision.Jan 8 2022, 6:17 AM

I am OK with this change. But it seems you need to rebase your code.

This revision is now accepted and ready to land.Jan 8 2022, 6:17 AM
This revision was landed with ongoing or failed builds.Jan 19 2022, 5:22 AM
This revision was automatically updated to reflect the committed changes.