Page MenuHomePhabricator

[AVR] Remove faulty stack pushing behavior
ClosedPublic

Authored by aykevl on Apr 21 2020, 12:32 PM.

Details

Summary

An instruction like this will need to allocate some stack space for the last parameter:

%x = call addrspace(1) i16 @bar(i64 undef, i64 undef, i16 undef, i16 0)

This worked fine when passing an actual value (in this case 0). However, when passing undef, no value was pushed to the stack and therefore no push instructions were created. This caused an unbalanced stack leading to interesting results.

This commit fixes that by replacing the push logic with a regular stack adjustment and stack-relative load/stores. This is less efficient but at least it correctly compiles the code.

I can think of a few improvements in the future:

  • The stack should have been adjusted in the function prologue when there are no allocas in the function.
  • Many (if not most) stack adjustments can be replaced by pushing/popping the values directly. Exactly like the previous code attempted but didn't do correctly.
  • Small stack adjustments can be done more efficiently with a few push/pop instructions (pushing/popping bogus values), both for code size and for speed.

All in all, as long as there are no allocas in the function I think that is almost always more efficient to emit regular push/pop instructions. This is however left for future optimizations.


Together with D78579 this fixes two additional tests in compiler-rt: powidf2_test.c and powixf2_test.c.

Diff Detail

Event Timeline

aykevl created this revision.Apr 21 2020, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 12:32 PM
Herald added subscribers: Jim, hiraditya. · View Herald Transcript
aykevl edited the summary of this revision. (Show Details)Apr 21 2020, 12:34 PM
dylanmckay accepted this revision.Jun 16 2020, 4:33 AM

This is good, this is good. Nice work @aykevl, approved.

This revision is now accepted and ready to land.Jun 16 2020, 4:33 AM
This revision was automatically updated to reflect the committed changes.

I have found that my fix isn't complete, it can result in broken code because the register allocator does not know that R31R30 is in use between ADJCALLSTACKDOWN and STDWSPQRr and might use it as the value operand for the STDWSPQRr instruction.

...I'm not sure how to fix this.

I have a fix ready and will post a patch soon.