This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix access to stack arguments when re-aligning SP in Armv6m
ClosedPublic

Authored by chill on Feb 21 2018, 4:18 AM.

Details

Summary

When an Armv6m function dynamically re-aligns the stack, access to incoming stack arguments (and to stack area, allocated for register varargs)
is done via SP, which is incorrect. For example, compiling:

void h(int, int *);
void f(int n, ...) {
  __builtin_va_list ap;
  __builtin_va_start(ap, n);
  __attribute__((aligned(16))) int v[4];
  h(n, v);
}

with clang -target arm-eabi -mcpu=cortex-m0 -O2 yields the following assembly:

f:
        sub     sp, #12
        push    {r4, r6, r7, lr}
        add     r7, sp, #8
        sub     sp, #20
        mov     r4, sp
        lsrs    r4, r4, #4
        lsls    r4, r4, #4
        mov     sp, r4
        str     r3, [sp, #44]
        str     r2, [sp, #40]
        str     r1, [sp, #36]
     ...

where incoming register varargs are stored using the SP after alignment.

This patch fixes it, by making access to "fixed" frame objects be done via FP when the function needs stack re-alignment.
It also changes the access to "fixed" frame objects be done via FP (instead of using R6/BP) also for the case when the stack frame contains variable sized objects. This should allow more objects to fit within the immediate offset of the load instruction.

All of the above via a small refactoring to reuse the existing ARMFrameLowering::ResolveFrameIndexReference.

Diff Detail

Repository
rL LLVM

Event Timeline

chill created this revision.Feb 21 2018, 4:18 AM

Would it be possible to use ResolveFrameIndexReference here, like ARMBaseRegisterInfo::eliminateFrameIndex does?

lib/Target/ARM/ThumbRegisterInfo.cpp
534 ↗(On Diff #135228)

Please add a comment explaining which objects must be referenced some particular way, and which objects you prefer to reference one way or the other for the sake of optimization.

543 ↗(On Diff #135228)

Please put a comment here briefly explaining why this special-case is necessary.

chill updated this revision to Diff 136287.Feb 28 2018, 6:11 AM
chill edited the summary of this revision. (Show Details)

Update:

  • refactor to use the existing ARMFrameLowering::ResolveFrameIndexReference.
  • small tweak to the latter to better accommodate T1.
This revision is now accepted and ready to land.Feb 28 2018, 12:03 PM
chill added a comment.Mar 1 2018, 2:12 AM

Thanks a lot for the review!

This revision was automatically updated to reflect the committed changes.