Page MenuHomePhabricator

[LivePhysRegs] Preserve pristine registers in blocks with no successors.
ClosedPublic

Authored by efriedma on Jan 24 2018, 3:11 PM.

Details

Summary

One common source of blocks with no successors is calls to noreturn functions; we want to preserve pristine registers in case the call throws an exception.

The whole pristine register thing is messy (it would be better if PEI marked liveness explicitly in the MIR), but this fills a hole in the model for now.

Fixes https://bugs.llvm.org/show_bug.cgi?id=36073.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Jan 24 2018, 3:11 PM

Oops, messed up the testcase; I'll fix it soon.

A noreturn call doesn't need to have the callers callee saved registers restored. I also think this fact shouldn't change with throwing calls (I assume the unwinding tables would contain information on how to restore the callee saves in this case?).

So I'm not convinced yet this change is correct.

efriedma updated this revision to Diff 131369.Jan 24 2018, 3:35 PM

Fix testcase so both functions use the same codepath.

Without this patch, LLVM trunk generates the following code:

no_stm:
        .fnstart
@ %bb.0:                                @ %entry
        .save   {r7, lr}
        push    {r7, lr}
        .setfp  r7, sp
        add     r7, sp, #0
        mov     r4, r3
        adds    r4, #8
        stm     r4!, {r0, r1, r2}
        bl      throws_2

How do you expect the unwinder to restore r4 after we've overwritten it?

FWIW I've tested this locally and it does indeed fix the issues we were seeing!

A noreturn call doesn't need to have the callers callee saved registers restored.

Why not? The non-returning call doesn't come back to its call site, but it goes somewhere. From the point of view of some function foo, all of that activity may happen inside the call to bar (which would have further calls in it). Not preserving non-volatile registers could cause them to end up being clobbered across the call to bar.

A noreturn call doesn't need to have the callers callee saved registers restored.

Why not? The non-returning call doesn't come back to its call site, but it goes somewhere. From the point of view of some function foo, all of that activity may happen inside the call to bar (which would have further calls in it). Not preserving non-volatile registers could cause them to end up being clobbered across the call to bar.

I think there's two concepts here:
a) A tailcall (i.e. instead of a "call X; return" combination you use "jmp X"). We use special tailcall instructions for this that have the isReturn flag set and mostly look like a normal return to regalloc.
b) A noreturn call (i.e. exit(), abort()). Those are normals calls that just happen to never return. This case looks like a normal case we just somehow have no terminator instruction in a block without any successor blocks.

We need to restore callee saves for case a) but should that anyway as it looks like a return. We could restore callee saves in case after the call in case b), but why bother: we will never get there anyway.

MatzeB accepted this revision.Jan 25 2018, 3:48 PM

Ok the subtle detail I missed before is that noreturn calls may still throw an exception/cause stack unwinding apparently. In that case we indeed need to prepare for CSR restoration.

So LGTM, thanks!

We could go back to the previous code in case the noreturn call is marked nothrows and safe a few spill instructions. But let's leave that for later and get it correct first.

Without this patch, LLVM trunk generates the following code:

no_stm:
        .fnstart
@ %bb.0:                                @ %entry
        .save   {r7, lr}
        push    {r7, lr}
        .setfp  r7, sp
        add     r7, sp, #0
        mov     r4, r3
        adds    r4, #8
        stm     r4!, {r0, r1, r2}
        bl      throws_2

How do you expect the unwinder to restore r4 after we've overwritten it?

This revision is now accepted and ready to land.Jan 25 2018, 3:48 PM
This revision was automatically updated to reflect the committed changes.

Oh before, have the exact same problem in LiveRegUnits::addLiveOuts(). I can take a look at that later if you don't beat me to it.

eastig added a subscriber: eastig.Jan 30 2018, 5:51 AM

Hi Eli,

The patch causes a compiler crash whilst compiling Spec2k 176.gcc/src/insn-attrtab.c on Linux Cortex-A9.
The clang command:

clang  -DNDEBUG  -O3 -DNDEBUG -ffast-math -mcpu=cortex-a9 -mthumb -fomit-frame-pointer   ""   -w -Werror=date-time -std=gnu89 -DSPEC_CPU2000 -D__DATE__=\"XXX\" -D__TIME__=\"XXX\" -c ./CINT2000/176.gcc/src/insn-attrtab.c

I am attaching a test created with bugpoint:

To reproduce:

$ llc -O3 -o /dev/null -filetype=asm test.ll
llc: ../lib/Target/ARM/Thumb2SizeReduction.cpp:965: bool UpdateCPSRUse(llvm::MachineInstr&, bool): Assertion `LiveCPSR && "CPSR liveness tracking is wrong!"' failed.
#0 0x0000000001fbbfba llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/work/repos/llvm01/build.b/bin/llc+0x1fbbfba)
#1 0x0000000001fba17e llvm::sys::RunSignalHandlers() (/work/repos/llvm01/build.b/bin/llc+0x1fba17e)
#2 0x0000000001fba2e0 SignalHandler(int) (/work/repos/llvm01/build.b/bin/llc+0x1fba2e0)
#3 0x00007fbbe63a5330 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)
#4 0x00007fbbe518dc37 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x36c37)
#5 0x00007fbbe5191028 abort (/lib/x86_64-linux-gnu/libc.so.6+0x3a028)
#6 0x00007fbbe5186bf6 (/lib/x86_64-linux-gnu/libc.so.6+0x2fbf6)
#7 0x00007fbbe5186ca2 (/lib/x86_64-linux-gnu/libc.so.6+0x2fca2)
#8 0x0000000000ba2814 (anonymous namespace)::Thumb2SizeReduce::runOnMachineFunction(llvm::MachineFunction&) (/work/repos/llvm01/build.b/bin/llc+0xba2814)
#9 0x0000000001747da5 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/work/repos/llvm01/build.b/bin/llc+0x1747da5)
#10 0x0000000001a5f3a3 llvm::FPPassManager::runOnFunction(llvm::Function&) (/work/repos/llvm01/build.b/bin/llc+0x1a5f3a3)
#11 0x0000000001a5f44c llvm::FPPassManager::runOnModule(llvm::Module&) (/work/repos/llvm01/build.b/bin/llc+0x1a5f44c)
#12 0x0000000001a5fcba llvm::legacy::PassManagerImpl::run(llvm::Module&) (/work/repos/llvm01/build.b/bin/llc+0x1a5fcba)
#13 0x00000000007dc358 compileModule(char**, llvm::LLVMContext&) (/work/repos/llvm01/build.b/bin/llc+0x7dc358)
#14 0x000000000076bd74 main (/work/repos/llvm01/build.b/bin/llc+0x76bd74)
#15 0x00007fbbe5178f45 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f45)
#16 0x00000000007d0726 _start (/work/repos/llvm01/build.b/bin/llc+0x7d0726)
Stack dump:
0.      Program arguments: /work/repos/llvm01/build.b/bin/llc -O3 -o /dev/null -filetype=asm test.ll
1.      Running pass 'Function Pass Manager' on module 'test.ll'.
2.      Running pass 'Thumb2 instruction size reduce pass' on function '@test'
Aborted

Thanks,
Evgeny Astigeevich

I think that issue will be fixed by https://reviews.llvm.org/D42655.