This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Store SP adjustment in MachineBasicBlock. NFCI.
ClosedPublic

Authored by foad on Jul 1 2023, 5:55 AM.

Details

Summary

Record the SP adjustment on entry to each basic block. This is almost
always zero except on targets like ARM which can split a basic block in
the middle of a call sequence.

This simplifies PEI::replaceFrameIndices which previously had to visit
basic blocks in a specific order and had special handling for
unreachable blocks. More importantly it paves the way for an equally
simple implementation of a backwards version of replaceFrameIndices,
which is required to fully convert PrologEpilogInserter to backwards
register scavenging, which is preferred because it does not rely on
accurate kill flags.

Diff Detail

Event Timeline

foad created this revision.Jul 1 2023, 5:55 AM
foad requested review of this revision.Jul 1 2023, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2023, 5:55 AM
foad added inline comments.Jul 1 2023, 6:35 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
118

On 64-bit hosts this fills a hole in the struct so does not increase the size of MachineBasicBlock.

arsenm added a subscriber: arsenm.Jul 1 2023, 6:59 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/MachineBasicBlock.h
118

Needs mir serialization

foad updated this revision to Diff 536534.Jul 1 2023, 9:47 AM

Add MIR serialization.

foad added inline comments.Jul 1 2023, 9:49 AM
llvm/lib/CodeGen/MIRParser/MIParser.cpp
685

This won't work for negative values but in practice this value is always >= 0 for ARM (and == 0 for all other in-tree targets). I don't know whether it could ever reasonably be negative on other targets.

arsenm added a comment.Jul 7 2023, 5:32 AM

Missing MIR parse/print tests

llvm/include/llvm/CodeGen/MachineBasicBlock.h
118

Also the llvm-reduce mir cloner should copy this

llvm/lib/Target/ARM/ARMISelLowering.cpp
11374
foad updated this revision to Diff 538407.Jul 9 2023, 1:32 AM

Update.

foad marked 2 inline comments as done.Jul 9 2023, 1:33 AM

Missing MIR parse/print tests

Added.

arsenm accepted this revision.Jul 11 2023, 12:25 PM
This revision is now accepted and ready to land.Jul 11 2023, 12:25 PM
This revision was automatically updated to reflect the committed changes.

Should the verifier check the block stored adjustment matches the calls in the block?

foad added a comment.Jul 12 2023, 8:20 AM

Should the verifier check the block stored adjustment matches the calls in the block?

That would be nice, and it would fit naturally in MachineVerifier::verifyStackFrame. But after PEI eliminates call frame setup instructions, you can no longer infer the SP adjustment by examining the instructions in a block. I don't know how to handle that case.

chapuni added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1350–1353

Only +Asserts?

Should the verifier check the block stored adjustment matches the calls in the block?

That would be nice, and it would fit naturally in MachineVerifier::verifyStackFrame. But after PEI eliminates call frame setup instructions, you can no longer infer the SP adjustment by examining the instructions in a block. I don't know how to handle that case.

I am tempted to revert this patch while I work on the machine verification aspect. I am having second thoughts about whether "sp adjustment" is the right abstraction to store in the basic block - maybe "frame size" or "frame total size" would make more sense.

Hi, this change has caused a regression in one of our tests:

#include <assert.h>
#include <alloca.h>
#include <stdio.h>

struct S51 {
  int M50[20];
};

struct S2 {
  int a, b;
};

struct S2 F38(struct S51 P4) {
  int V0 = 14;
  __asm("" : "+r"(V0));
  char *V1 = (char *)alloca(V0);
  __asm volatile("" : : "r"(V1));

  struct S2 result = {1, 2};
  return result;
}

signed int main(void) {
  struct S51 P4 = {0};
  struct S2 ret = F38(P4);

  assert(ret.a == 1);
  assert(ret.b == 2);

  return 0;
}
$ /work/ac6/build-armclang/bin/clang --target=arm--none-eabi -march=armv7-m -c test.c -O1 -o - -S
...
main:
        .fnstart
@ %bb.0:                                @ %entry
        .save   {r4, r6, r7, lr}
        push    {r4, r6, r7, lr}
        .setfp  r7, sp, #8
        add     r7, sp, #8
        .pad    #160
        sub     sp, #160
        add     r4, sp, #80
        mov     r0, r4
        movs    r1, #80
        bl      __aeabi_memclr4
        mov     r0, sp
        add.w   r1, r4, #12
        movs    r2, #68
.LBB1_1:                                @ %entry
                                        @ =>This Inner Loop Header: Depth=1
        ldr     r3, [r1], #4
        subs    r2, #4
        str     r3, [r0], #4
        bne     .LBB1_1
@ %bb.2:                                @ %entry
        add     r3, sp, #152
        ldm     r3, {r1, r2, r3}
        add     r0, sp, #144
        bl      F38
        ldr     r0, [sp, #144]
        cmp     r0, #1
        bne     .LBB1_5
@ %bb.3:                                @ %cond.end
        ldr     r0, [sp, #76]
        cmp     r0, #2
        ittt    eq
        moveq   r0, #0
        addeq   sp, #160
        popeq   {r4, r6, r7, pc}
.LBB1_4:                                @ %cond.false5
        movw    r0, :lower16:.L.str.2
        movw    r1, :lower16:.L.str.1
        movt    r0, :upper16:.L.str.2
        movt    r1, :upper16:.L.str.1
        movs    r2, #30
        bl      __aeabi_assert
.LBB1_5:                                @ %cond.false
        movw    r0, :lower16:.L.str
        movw    r1, :lower16:.L.str.1
        movt    r0, :upper16:.L.str
        movt    r1, :upper16:.L.str.1
        movs    r2, #29
        bl      __aeabi_assert
.Lfunc_end1:
...

The bug is in the code generated for main: according to the ARM ABI, the return type of F38 should be passed in memory provided by the caller, with a pointer to it passed in r0. The code for main sets this to SP+144, and reads the first member of the struct from that address, but then reads the second member from SP+76, instead of SP+148.

foad added a comment.Jul 13 2023, 6:36 AM

@olista01 thanks for the test case! It would be great if there were more like this in the existing lit tests :)

luporl added a subscriber: luporl.Jul 13 2023, 7:21 AM

Just for the record, these buildbots were also failing, in llvm-test-suite, with this patch:
https://lab.llvm.org/buildbot/#/builders/174/builds/22768
https://lab.llvm.org/buildbot/#/builders/186/builds/10783

They are passing again now.

foad added a comment.Jul 13 2023, 11:00 AM

@luporl thanks, I will check that before trying to reland anything.