Page MenuHomePhabricator

[GC] Remove buggy untested optimization from statepoint lowering
ClosedPublic

Authored by reames on Mar 10 2020, 3:59 PM.

Details

Summary

A downstream test case (still in the process of being properly reduced, will be added here) revealed that we have a bug in how we handle duplicate relocations. If we have the same SDValue relocated twice, and that value happens to be a constant (such as null), we only export one of the two llvm::Values. Exporting on a per llvm::Value basis is required to allow lowering of gc.relocates in following basic blocks (e.g. invokes). Without it, we end up with a use of an undefined vreg and bad things happen.

Rather than fixing the optimization - which appears to be hard - I propose we simply remove it. There are no tests in tree that change with this code removed. If we find out later that this did matter for something, we can reimplement a variation of this in CodeGenPrepare to catch the easy cases without complicating the lowering code.

Thanks to Denis and Serguei who did all the hard work of figuring out what went wrong here. The patch is by far the easy part. :)

Diff Detail

Event Timeline

reames created this revision.Mar 10 2020, 3:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 3:59 PM
skatkov added a subscriber: test.EditedMar 10 2020, 8:29 PM

Here is a simple test showing the problem:

; RUN: llc < %s -verify-machineinstrs

declare void @func()

define i1 @test() gc "statepoint-example" {
entry:
  %safepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @func, i32 0, i32 0, i32 0, i32 0, i32 addrspace(1)* null, i32 addrspace(1)* null)
  %base = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %safepoint_token,  i32 7, i32 7)
  %derived = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %safepoint_token,  i32 7, i32 8)
  %safepoint_token2 = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @func, i32 0, i32 0, i32 0, i32 0, i32 addrspace(1)* %base, i32 addrspace(1)* %derived)
  br label %next

next:
  %base_reloc = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %safepoint_token2,  i32 7, i32 7)
  %derived_reloc = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %safepoint_token2,  i32 7, i32 8)
  %cmp1 = icmp eq i32 addrspace(1)* %base_reloc, null
  %cmp2 = icmp eq i32 addrspace(1)* %derived_reloc, null
  %cmp = and i1 %cmp1, %cmp2
  ret i1 %cmp
}

declare token @llvm.experimental.gc.statepoint.p0f_isVoidf(i64, i32, void ()*, i32, i32, ...)
declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token, i32, i32) #3

without this patch I get the following crash (which disappears with it):

# After Instruction Selection
# Machine code for function test: IsSSA, TracksLiveness

bb.0.entry:
  successors: %bb.1(0x80000000); %bb.1(100.00%)

  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
  STATEPOINT 0, 0, 0, @func, 2, 0, 2, 0, 2, 0, 2, 0, 2, 0, <regmask $bh $bl $bp $bph $bpl $bx $ebp $ebx $hbp $hbx $rbp $rbx $r12 $r13 $r14 $r15 $r12b $r13b $r14b $r15b $r12bh $r13bh $r14bh $r15bh $r12d $r13d $r14d $r15d $r12w $r13w $r14w $r15w $r12wh and 3 more...>, implicit-def $rsp, implicit-def $ssp
  ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
  STATEPOINT 0, 0, 0, @func, 2, 0, 2, 0, 2, 0, 2, 0, 2, 0, <regmask $bh $bl $bp $bph $bpl $bx $ebp $ebx $hbp $hbx $rbp $rbx $r12 $r13 $r14 $r15 $r12b $r13b $r14b $r15b $r12bh $r13bh $r14bh $r15bh $r12d $r13d $r14d $r15d $r12w $r13w $r14w $r15w $r12wh and 3 more...>, implicit-def $rsp, implicit-def $ssp
  ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
  %1:gr32 = MOV32r0 implicit-def dead $eflags
  %0:gr64 = SUBREG_TO_REG 0, killed %1:gr32, %subreg.sub_32bit

bb.1.next:
; predecessors: %bb.0

  TEST64rr %2:gr64, %2:gr64, implicit-def $eflags
  %3:gr8 = SETCCr 4, implicit $eflags
  $al = COPY %3:gr8
  RET 0, $al

# End machine code for function test.

*** Bad machine code: Reading virtual register without a def ***
- function:    test
- basic block: %bb.1 next (0x1dc1350)
- instruction: TEST64rr %2:gr64, %2:gr64, implicit-def $eflags
- operand 0:   %2:gr64

*** Bad machine code: Reading virtual register without a def ***
- function:    test
- basic block: %bb.1 next (0x1dc1350)
- instruction: TEST64rr %2:gr64, %2:gr64, implicit-def $eflags
- operand 1:   %2:gr64
LLVM ERROR: Found 2 machine code errors.
Stack dump:
0.      Program arguments: /localhome/skatkov/work/llvm/build/buildDA/bin/llc -verify-machineinstrs
1.      Running pass 'Function Pass Manager' on module '<stdin>'.
2.      Running pass 'Verify generated machine code' on function '@test'
 #0 0x00007fb96b956360 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /localhome/skatkov/work/llvm/src/llvm/lib/Support/Unix/Signals.inc:564:0
 #1 0x00007fb96b9563f3 PrintStackTraceSignalHandler(void*) /localhome/skatkov/work/llvm/src/llvm/lib/Support/Unix/Signals.inc:625:0
 #2 0x00007fb96b954484 llvm::sys::RunSignalHandlers() /localhome/skatkov/work/llvm/src/llvm/lib/Support/Signals.cpp:68:0
 #3 0x00007fb96b955d9f SignalHandler(int) /localhome/skatkov/work/llvm/src/llvm/lib/Support/Unix/Signals.inc:406:0
 #4 0x00007fb968cc25d0 __restore_rt (/lib64/libpthread.so.0+0xf5d0)
 #5 0x00007fb96806b2c7 raise (/lib64/libc.so.6+0x362c7)
 #6 0x00007fb96806c9b8 abort (/lib64/libc.so.6+0x379b8)
 #7 0x00007fb96b80f9ea llvm::install_bad_alloc_error_handler(void (*)(void*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool), void*) /localhome/skatkov/work/llvm/src/llvm/lib/Support/ErrorHandling.cpp:130:0
 #8 0x00007fb96bffe29d (anonymous namespace)::MachineVerifierPass::runOnMachineFunction(llvm::MachineFunction&) /localhome/skatkov/work/llvm/src/llvm/lib/CodeGen/MachineVerifier.cpp:294:0
dantrushin accepted this revision.Mar 10 2020, 11:18 PM

LGTM.
I think, doing this in CodeGenPrepare is a good idea.

This revision is now accepted and ready to land.Mar 10 2020, 11:18 PM
This revision was automatically updated to reflect the committed changes.