Page MenuHomePhabricator

Remove TwoAddressInstructinoPass::sink3AddrInstruction.
ClosedPublic

Authored by jyknight on Jul 13 2020, 11:51 AM.

Details

Summary

This function has a bug which will incorrectly reschedule instructions
after an INLINEASM_BR (which can branch). (The bug may also allow
scheduling past a throwing-CALL, I'm not certain.)

I could fix that bug, but, as the removed FIXME noted, attempting to
move instructions _before_ converting to 3-addr, so as to avoid
needing to make the conversion in the first place is a better
idea. And, in fact, the code to do such reodering was added to this
pass in 2011, via the addition of the function rescheduleMIBelowKill,
only a few months after the FIXME was added. That reschedule code
does not contain the same bug.

The removal of this funciton is not a no-op: in some cases
sink3AddrInstruction would move an instruction after converting it to
3-addr form, when rescheduleMIBelowKill would not move before
converison. However, this does not appear to be important: the
machine instruction scheduler can reorder the after-conversion
instructions, in any case.

Diff Detail

Event Timeline

jyknight created this revision.Jul 13 2020, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2020, 11:51 AM
jyknight edited the summary of this revision. (Show Details)Jul 13 2020, 12:01 PM
nickdesaulniers marked an inline comment as done.Jul 13 2020, 12:38 PM

LGTM; tests pass and kernel panic is fixed. Just minor nits on things we might be able to simplify further in the test.

llvm/test/CodeGen/X86/callbr-asm-sink.ll
9

Looks like we only access the third element of this struct in the GEP, IIUC the GEP correctly?

25

I think you can drop the constant i32 1 from the callbr and its er, flags.

35

ret void?

llvm/test/CodeGen/X86/reverse_branches.ll
69

This move into %rdi after the call to _memchr is curious to me. Doesn't the x86_64 ABI that darwin uses use %rdi as the first parameter to a function call? Before hand, it looks like we're setting up the registers for the call as part of the calling convention. After, it looks like we're maybe misplacing the mov into %rdi?

If we jump to LBB0_5, we kill %rdi immediately.

Ah, LBB0_1 will have loaded %r15 into %rdi for LBB0_3.

Ok, I think this is correct, but would appreciate others to pay careful attention to this case.

I could fix that bug, but, as the removed FIXME noted, attempting to
move instructions _before_ converting to 3-addr, so as to avoid
needing to make the conversion in the first place is a better
idea.

Also, that sentence has more than the legally allowed number of commas. Please consider breaking it up into multiple sentences. Maybe:

I could fix that bug but as the removed FIXME noted, attempting to
move instructions _before_ converting to 3-addr so as to avoid
needing to make the conversion in the first place, is a better
idea.

craig.topper added inline comments.Jul 13 2020, 1:12 PM
llvm/test/CodeGen/X86/twoaddr-lea.ll
71–73

What's happening here? Previously the two instructions used the same register, but now they don't?

jyknight marked an inline comment as done.Jul 13 2020, 1:26 PM
jyknight added inline comments.
llvm/test/CodeGen/X86/twoaddr-lea.ll
71–73

Correct.

 _ham:                                   ## @ham
        .cfi_startproc
 ## %bb.0:                               ## %bb
        xorl    %r8d, %r8d
        movq    _global@GOTPCREL(%rip), %rdx
        movq    _global2@GOTPCREL(%rip), %rsi
        xorl    %eax, %eax
        cmpl    $10, %eax
        jle     LBB3_2
 LBB3_6:                                 ## %bb2
                                         ## =>This Loop Header: Depth=1
                                         ##     Child Loop BB3_7 Depth 2
        movl    (%rdx), %edi
        leal    (%rdi,%rax), %ecx
        movslq  %ecx, %rcx
 LBB3_7:                                 ## %bb6
                                         ##   Parent Loop BB3_6 Depth=1
                                         ## =>  This Inner Loop Header: Depth=2
        movq    %rax, (%rsi)
        movq    %rcx, (%rsi)
        movl    %edi, (%rdx)
        testb   %r8b, %r8b
        jne     LBB3_7
 ## %bb.8:                               ## %bb9
                                         ##   in Loop: Header=BB3_6 Depth=1
        addq    $4, %rax
        cmpl    $10, %eax
        jg      LBB3_6
 LBB3_2:                                 ## %bb3.preheader
        xorl    %ecx, %ecx
 LBB3_3:                                 ## %bb3
                                         ## =>This Loop Header: Depth=1
                                         ##     Child Loop BB3_4 Depth 2
-       movl    %eax, %edx
-       subl    %ecx, %edx
-       movq    %rcx, %rsi
+       movq    %rcx, %rdx
        addq    $4, %rcx
+       movl    %eax, %esi
+       subl    %edx, %esi
 LBB3_4:                                 ## %bb4
                                         ##   Parent Loop BB3_3 Depth=1
                                         ## =>  This Inner Loop Header: Depth=2
-       testl   %edx, %edx
+       testl   %esi, %esi
        jne     LBB3_9
 ## %bb.5:                               ## %bb5
                                         ##   in Loop: Header=BB3_4 Depth=2
-       incq    %rsi
-       cmpq    %rcx, %rsi
+       incq    %rdx
+       cmpq    %rcx, %rdx
        jge     LBB3_3
        jmp     LBB3_4
 LBB3_9:                                 ## %bb8
        ud2
jyknight updated this revision to Diff 277560.Jul 13 2020, 1:53 PM
jyknight marked 5 inline comments as done.

Improve commit message wording
Simplify added test a bit

llvm/test/CodeGen/X86/callbr-asm-sink.ll
9

Yeah. Needs to be a non-zero offset but other than that doesn't matter; I can get rid of the middle elements.

llvm/test/CodeGen/X86/callbr-asm-sink.ll
25

what is $2 in this case if there's only 2 parameters explicitly passed into the asm? Maybe you can drop that here and in the CHECK-NEXT above?

jyknight marked an inline comment as done.Jul 14 2020, 11:50 AM
jyknight added inline comments.
llvm/test/CodeGen/X86/callbr-asm-sink.ll
25

Oops, that was an oversight, I'll fix before pushing. I'm rather surprised this isn't an error -- who knew you could index into the clobber arguments (other than "memory"), in LLVM asm!

nickdesaulniers accepted this revision.Jul 15 2020, 10:27 AM

please don't forget to remove the $2

This revision is now accepted and ready to land.Jul 15 2020, 10:27 AM
void accepted this revision.Jul 15 2020, 1:05 PM

LGTM

This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Jul 17 2020, 2:06 AM

This might be a good candidate for release/11.x. What do you think?

hans added a comment.Jul 20 2020, 2:21 AM

This might be a good candidate for release/11.x. What do you think?

James pushed it to 11.x as 8a438096ffa48dadeb73b78844c53a7428aaec20