This is an archive of the discontinued LLVM Phabricator instance.

Differential D139756

[X86] Fix cmp order in probing BuildStackAlignAND
ClosedPublic

Authored by cuviper on Dec 9 2022, 5:13 PM.

Details

Reviewers
serge-sans-paille
Commits
rG9b8fcd04ef64: [X86] Fix cmp order in probing BuildStackAlignAND
Summary

Due to reversed arguments, the loop start was almost always skipping the
whole loop, since FinalStackProbed is probably less than StackPtr for
large alignments. The intent was to skip the loop if the first sub on
StackPtr made it less than FinalStackProbed already, so flip it.

Diff Detail

Event Timeline

cuviper created this revision.Dec 9 2022, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2022, 5:13 PM
Herald added subscribers: pengfei, hiraditya. · View Herald Transcript
cuviper requested review of this revision.Dec 9 2022, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2022, 5:13 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B202340: Diff 481795.Dec 9 2022, 6:09 PM
serge-sans-paille accepted this revision.Dec 13 2022, 11:45 AM

Thanks for spotting this! Reading the generated assembly again, we were indeed jumping over the actual loop instead of short-circuiting it...

This revision is now accepted and ready to land.Dec 13 2022, 11:45 AM
Closed by commit rG9b8fcd04ef64: [X86] Fix cmp order in probing BuildStackAlignAND (authored by cuviper). · Explain WhyDec 13 2022, 12:11 PM
This revision was automatically updated to reflect the committed changes.
cuviper added a commit: rG9b8fcd04ef64: [X86] Fix cmp order in probing BuildStackAlignAND.

Revision Contents

PathSize
llvm/
lib/
Target/
X86/
6 lines
test/
CodeGen/
X86/
2 lines
2 lines

Diff 482586

llvm/lib/Target/X86/X86FrameLowering.cpp

Show First 20 Lines • Show All 1,287 Lines • ▼ Show 20 Lines if (Reg == StackPtr && EmitInlineStackProbe && MaxAlign >= StackProbeSize) {
getSUBriOpcode(Uses64BitFramePtr, StackProbeSize); getSUBriOpcode(Uses64BitFramePtr, StackProbeSize);
BuildMI(headMBB, DL, TII.get(SUBOpc), StackPtr) BuildMI(headMBB, DL, TII.get(SUBOpc), StackPtr)
.addReg(StackPtr) .addReg(StackPtr)
.addImm(StackProbeSize) .addImm(StackProbeSize)
.setMIFlag(MachineInstr::FrameSetup); .setMIFlag(MachineInstr::FrameSetup);
BuildMI(headMBB, DL, BuildMI(headMBB, DL,
TII.get(Uses64BitFramePtr ? X86::CMP64rr : X86::CMP32rr)) TII.get(Uses64BitFramePtr ? X86::CMP64rr : X86::CMP32rr))
.addReg(FinalStackProbed)
.addReg(StackPtr) .addReg(StackPtr)
.addReg(FinalStackProbed)
.setMIFlag(MachineInstr::FrameSetup); .setMIFlag(MachineInstr::FrameSetup);
// jump // jump to the footer if StackPtr < FinalStackProbed
BuildMI(headMBB, DL, TII.get(X86::JCC_1)) BuildMI(headMBB, DL, TII.get(X86::JCC_1))
.addMBB(footMBB) .addMBB(footMBB)
.addImm(X86::COND_B) .addImm(X86::COND_B)
.setMIFlag(MachineInstr::FrameSetup); .setMIFlag(MachineInstr::FrameSetup);
headMBB->addSuccessor(bodyMBB); headMBB->addSuccessor(bodyMBB);
headMBB->addSuccessor(footMBB); headMBB->addSuccessor(footMBB);
} }
Show All 15 Lines if (Reg == StackPtr && EmitInlineStackProbe && MaxAlign >= StackProbeSize) {
// cmp with stack pointer bound // cmp with stack pointer bound
BuildMI(bodyMBB, DL, BuildMI(bodyMBB, DL,
TII.get(Uses64BitFramePtr ? X86::CMP64rr : X86::CMP32rr)) TII.get(Uses64BitFramePtr ? X86::CMP64rr : X86::CMP32rr))
.addReg(FinalStackProbed) .addReg(FinalStackProbed)
.addReg(StackPtr) .addReg(StackPtr)
.setMIFlag(MachineInstr::FrameSetup); .setMIFlag(MachineInstr::FrameSetup);
// jump // jump back while FinalStackProbed < StackPtr
BuildMI(bodyMBB, DL, TII.get(X86::JCC_1)) BuildMI(bodyMBB, DL, TII.get(X86::JCC_1))
.addMBB(bodyMBB) .addMBB(bodyMBB)
.addImm(X86::COND_B) .addImm(X86::COND_B)
.setMIFlag(MachineInstr::FrameSetup); .setMIFlag(MachineInstr::FrameSetup);
bodyMBB->addSuccessor(bodyMBB); bodyMBB->addSuccessor(bodyMBB);
bodyMBB->addSuccessor(footMBB); bodyMBB->addSuccessor(footMBB);
} }
▲ Show 20 Lines • Show All 2,540 Lines • Show Last 20 Lines

llvm/test/CodeGen/X86/stack-clash-large-large-align.ll

Show All 38 Lines
; CHECK-NEXT: movq %rsp, %rbp ; CHECK-NEXT: movq %rsp, %rbp
; CHECK-NEXT: .cfi_def_cfa_register %rbp ; CHECK-NEXT: .cfi_def_cfa_register %rbp
; CHECK-NEXT: movq %rsp, %r11 ; CHECK-NEXT: movq %rsp, %r11
; CHECK-NEXT: andq $-4096, %r11 # imm = 0xF000 ; CHECK-NEXT: andq $-4096, %r11 # imm = 0xF000
; CHECK-NEXT: cmpq %rsp, %r11 ; CHECK-NEXT: cmpq %rsp, %r11
; CHECK-NEXT: je .LBB1_4 ; CHECK-NEXT: je .LBB1_4
; CHECK-NEXT: # %bb.1: ; CHECK-NEXT: # %bb.1:
; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000 ; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000
; CHECK-NEXT: cmpq %rsp, %r11 ; CHECK-NEXT: cmpq %r11, %rsp
; CHECK-NEXT: jb .LBB1_3 ; CHECK-NEXT: jb .LBB1_3
; CHECK-NEXT: .LBB1_2: # =>This Inner Loop Header: Depth=1 ; CHECK-NEXT: .LBB1_2: # =>This Inner Loop Header: Depth=1
; CHECK-NEXT: movq $0, (%rsp) ; CHECK-NEXT: movq $0, (%rsp)
; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000 ; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000
; CHECK-NEXT: cmpq %rsp, %r11 ; CHECK-NEXT: cmpq %rsp, %r11
; CHECK-NEXT: jb .LBB1_2 ; CHECK-NEXT: jb .LBB1_2
; CHECK-NEXT: .LBB1_3: ; CHECK-NEXT: .LBB1_3:
; CHECK-NEXT: movq %r11, %rsp ; CHECK-NEXT: movq %r11, %rsp
Show All 27 Lines

llvm/test/CodeGen/X86/stack-clash-small-large-align.ll

Show All 34 Lines
; CHECK-NEXT: movq %rsp, %rbp ; CHECK-NEXT: movq %rsp, %rbp
; CHECK-NEXT: .cfi_def_cfa_register %rbp ; CHECK-NEXT: .cfi_def_cfa_register %rbp
; CHECK-NEXT: movq %rsp, %r11 ; CHECK-NEXT: movq %rsp, %r11
; CHECK-NEXT: andq $-65536, %r11 # imm = 0xFFFF0000 ; CHECK-NEXT: andq $-65536, %r11 # imm = 0xFFFF0000
; CHECK-NEXT: cmpq %rsp, %r11 ; CHECK-NEXT: cmpq %rsp, %r11
; CHECK-NEXT: je .LBB1_4 ; CHECK-NEXT: je .LBB1_4
; CHECK-NEXT: # %bb.1: ; CHECK-NEXT: # %bb.1:
; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000 ; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000
; CHECK-NEXT: cmpq %rsp, %r11 ; CHECK-NEXT: cmpq %r11, %rsp
; CHECK-NEXT: jb .LBB1_3 ; CHECK-NEXT: jb .LBB1_3
; CHECK-NEXT: .LBB1_2: # =>This Inner Loop Header: Depth=1 ; CHECK-NEXT: .LBB1_2: # =>This Inner Loop Header: Depth=1
; CHECK-NEXT: movq $0, (%rsp) ; CHECK-NEXT: movq $0, (%rsp)
; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000 ; CHECK-NEXT: subq $4096, %rsp # imm = 0x1000
; CHECK-NEXT: cmpq %rsp, %r11 ; CHECK-NEXT: cmpq %rsp, %r11
; CHECK-NEXT: jb .LBB1_2 ; CHECK-NEXT: jb .LBB1_2
; CHECK-NEXT: .LBB1_3: ; CHECK-NEXT: .LBB1_3:
; CHECK-NEXT: movq %r11, %rsp ; CHECK-NEXT: movq %r11, %rsp
Show All 24 Lines