Page MenuHomePhabricator

[RISCV] Handle variable sized objects with the stack need to be realigned
ClosedPublic

Authored by shiva0217 on Oct 15 2019, 3:03 AM.

Details

Summary

To handle the variable-sized objects with stack alignment, we need another base register(BP) to record the SP value after realignment. So we could reference the local variables after allocating the variable-sized objects by the base register. To avoid the BP value clobbered by a function call, we need to choose a callee saved register as BP. RV32E only has X8 and X9 as callee saved registers and X8 will be used as FP. So X9 will be chosen as the BP register.

Diff Detail

Event Timeline

shiva0217 created this revision.Oct 15 2019, 3:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2019, 3:03 AM

This is looking good. Just a few small comments at the moment, and I'm going to kick off a testing run on our internal build system.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
327

Please may you use getBPReg here instead of hard-coded RISCV::X9?

At some point, we should use getFPReg for everywhere we hard-code RISCV::X2, but that's irrelevant to this change.

353

Again, use getBPReg here to avoid hard-coding RISCV::X9.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
92

Again, use getBPReg here (if you can, this one is harder)

Hi Shiva, I will evaluate your patch today and report back.

Shiva, I have tried a few workloads like EEMBC, SPEC2000/2006, perennial c++, plumhall.
They don't seem to use variable length arrays nor allocas to test this patch.
Which test suite are you using?

Shiva, I have tried a few workloads like EEMBC, SPEC2000/2006, perennial c++, plumhall.
They don't seem to use variable length arrays nor allocas to test this patch.
Which test suite are you using?

Hi Ana,

Thanks for the work. I ran GCC testsuite and gcc.c-torture/compile/pr51354.c will trigger the patch.
I expect rv32e would have little more cases trigger the patch due to smaller stack alignment.
But the cases of variable-sized objects with realignment are not very often.

shiva0217 updated this revision to Diff 225571.EditedOct 18 2019, 12:20 AM

Update patch to address the comments.

Hi @lenary, thanks for the review

luismarques added inline comments.Oct 24 2019, 8:27 AM
llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll
10

This should also be tested with FP and CFI directives.

shiva0217 marked an inline comment as done.Oct 27 2019, 6:52 PM
shiva0217 added inline comments.
llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll
10

For the frame contain variable-sized objects, hasFP() will return true. Ok, I'll add CFI checking line, thanks.

shiva0217 updated this revision to Diff 226609.Oct 27 2019, 6:54 PM

Add CFI checking line in the test case

lenary added inline comments.Oct 28 2019, 7:12 AM
llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll
34

This doesn't look right. Why does this directive come after the restore of s0 and s1 (but not ra)? Shouldn't it come immediately after the addi instruction?

shiva0217 marked an inline comment as done.Oct 28 2019, 8:22 PM
shiva0217 added inline comments.
llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll
34

I think the idea is that before s0 been restored, debugger could use s0 as the base to calculate CFA. After the s0 been corrupted, the CFA calculation has to base on sp with sp_offset. addi sp, s0, -128 will be generated for the frame contains variable-sized objects. For the case has FP but not contains variable-sized objects, the directive need to be inserted, too. I guess that's why the directive inserts here.

Please can you rebase this on top of master? @luismarques has removed CFI directives from function epilogs (because they were sometimes buggy), which should make this code simpler.

Rebase to the master

Hi @lenary, thanks for the reminding.

lenary accepted this revision.Nov 15 2019, 3:36 AM

LGTM, Let's get this merged!

This revision is now accepted and ready to land.Nov 15 2019, 3:36 AM
This revision was automatically updated to reflect the committed changes.