This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix using the SJLJ jump table on x86_64
ClosedPublic

Authored by mstorsjo on Sep 27 2017, 2:13 PM.

Details

Summary

The previous version didn't work if the jump table base address didn't fit in 32 bit, since it was encoded as an immediate offset.

And in case the jump table is encoded as 32 bit label differences, we need to load and add them to the table base first.

This solves the first half of the issues mentioned in PR34720.

Also fix some of the errors pointed out by -verify-machineinstrs, by using GR32_NOSPRegClass.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 27 2017, 2:13 PM
mstorsjo updated this revision to Diff 116932.Sep 28 2017, 12:33 AM
mstorsjo retitled this revision from [X86] Load the SJLJ jump table address into a register on x86_64 to [X86] Fix using the SJLJ jump table on x86_64.
mstorsjo edited the summary of this revision. (Show Details)

Handled the case of MachineJumpTableInfo::EK_LabelDifference32 as well, which is what the jump table ends up as on win64/x86.

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
test/CodeGen/X86/sjlj-eh.ll
3

According to PR27481, sjlj-eh.ll fails EXPENSIVE_CHECKS builds - please can you add "-verify-machineinstrs" to all the llc commands?

mstorsjo added inline comments.Sep 28 2017, 2:20 AM
test/CodeGen/X86/sjlj-eh.ll
3

Sure, I'll try to look into that and see if I'm capable of figuring out what to fix to silence those issues, while digging into this.

mstorsjo added inline comments.Sep 28 2017, 1:38 PM
lib/Target/X86/X86ISelLowering.cpp
26652

If building with optimizations, this works as intended. If building without optimizations, this actually uses another, uninitialized register here instead of IReg expanded to 64 bit (as done with TargetOpcode::COPY above). Does anybody happen to have a hint about why that happens?

mstorsjo added inline comments.Sep 28 2017, 1:59 PM
lib/Target/X86/X86ISelLowering.cpp
26652

In particular, this issue seems to happen when the value gets spilled onto the stack. Originally the value (IReg) was loaded in eax, but then was spilled onto the stack, reloaded in ecx, but then IReg64 is used as rdx, which actually doesn't contain the intended value.

So without optimization, this gets compiled into:

        movl    -304(%rbp), %eax
        cmpl    $9, %eax
        movl    %eax, -448(%rbp)        # 4-byte Spill
        jae     .LBB4_26
# BB#25:                                #   in Loop: Header=BB4_24 Depth=1
        leaq    .LJTI4_0(%rip), %rax    
        movl    -448(%rbp), %ecx        # 4-byte Reload - into ecx now
        movl    (%rax,%rdx,4), %ecx   # This uses rdx instead of rcx which contains the value we want
        movslq  %ecx, %rdx
        addq    %rax, %rdx
        movl    %ecx, -448(%rbp)        # 4-byte Spill
        jmpq    *%rdx
mstorsjo updated this revision to Diff 117123.Sep 29 2017, 4:56 AM

Fixed the issues in unoptimized builds; it took some time to figure out that I couldn't use TargetOpcode::COPY to zero-extend a 32 bit register into 64 bit, but had to use TargetOpcode::SUBREG_TO_REG. Also now using more virtual registers for each intermediate value instead of reusing them - this seemed to produce more sensible annotated output from different register allocators (and doesn't matter when using optimizations properly). But if you prefer, I can reduce it down to just using as many virtual registers as before.

mstorsjo updated this revision to Diff 117125.Sep 29 2017, 5:05 AM
mstorsjo edited the summary of this revision. (Show Details)

Also get rid of some of the errors with -verify-machineinstrs.

These remain though, that are unrelated to this patch:

*** Bad machine code: MBB has allocatable live-in, but isn't entry or landing-pad. ***  
- function:    _Z8functionv
- basic block: BB#1 lpad (0x2dedac0)
compnerd added inline comments.Oct 2 2017, 7:16 PM
lib/Target/X86/X86ISelLowering.cpp
26571

Why the restriction on SP?

26594

I think it might be better to handle this as a switch.

26603

OReg might be better to identify that this is a register, not the actual offset.

26604

OReg64 might be better to identify that this is the register, not the offset.

26605

TReg might be better to identify that this the register, not the actual target address.

26617

I think that this might be complex enough to warrant a bit of MIR in a comment to allow a quicker read of what you are trying to construct.

26622

Aren't we guaranteed that Subtarget.is64Bit() is false here due to the condition to which this else clause belongs is !Subtarget.is64Bit().

test/CodeGen/X86/sjlj-eh.ll
121

Am I not understanding something here? This looks more like *Handlers[UFC.__callsite + 1] as you are indexing by %rax + 4.

127

Can you please check the entire sequence for the dispatch?

mstorsjo marked 4 inline comments as done.Oct 2 2017, 11:13 PM
mstorsjo added inline comments.
lib/Target/X86/X86ISelLowering.cpp
26571

If building with -verify-machineinstrs, the verifier will point out that this register is used as an index in a memory operand like (base+IReg*4), and that register can't be of the GR32 class, it has to be GR32_NOSP.

26617

Sure, I'll add comments with what the expected instructions look like

26622

Oh, indeed - I fixed a few other similar ones but apparently missed this one.

test/CodeGen/X86/sjlj-eh.ll
121

No, this does eax = *(rcx + 4*rax)

127

Sure, can do

mstorsjo updated this revision to Diff 117474.Oct 2 2017, 11:19 PM

Changed the if to a switch, added comments about what instructions are supposed to be generated, removed the superfluous conditional in the 32 bit codepath, expanded the test for x86_64 linux, renamed register variables as suggested (and renamed Base into BReg).

compnerd accepted this revision.Oct 3 2017, 1:53 PM

Please adjust the comments.

lib/Target/X86/X86ISelLowering.cpp
26571

Would be nice to get a comment that indicates that because the register is used as an index into a memory operand, SP is not valid.

26624

%rcx is not guaranteed, BReg would be better.

26631

%rax and %eax are not guaranteed.

26639

%rcx and %rax are not guaranteed, BReg and IReg would be better.

26651

Please add a newline before this.

%rcx and %rax are not guaranteed, BReg and IReg would be better.

26658

%rax, %eax is not guaranteed, OReg64 and OReg would be better.

26661

%rcx, %rax -> TReg, OReg64, BReg.

26665

%rax -> TReg.

26673

%eax -> IReg.

This revision is now accepted and ready to land.Oct 3 2017, 1:53 PM
This revision was automatically updated to reflect the committed changes.