Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[MachineBlockPlacement] Don't make blocks "uneditable"
Summary: This fixes an issue with MachineBlockPlacement due to a badly timed call to `analyzeBranch` with `AllowModify` set to true. The timeline is as follows: 1. `MachineBlockPlacement::maybeTailDuplicateBlock` calls `TailDup.shouldTailDuplicate` on its argument, which in turn calls `analyzeBranch` with `AllowModify` set to true. 2. This `analyzeBranch` call edits the terminator sequence of the block based on the physical layout of the machine function, turning an unanalyzable non-fallthrough block to a unanalyzable fallthrough block. Normally MBP bails out of rearranging such blocks, but this block was unanalyzable non-fallthrough (and thus rearrangeable) the first time MBP looked at it, and so it goes ahead and decides where it should be placed in the function. 3. When placing this block MBP fails to analyze and thus update the block in keeping with the new physical layout. Concretely, before (1) we have something like: ``` LBL0: < unknown terminator op that may branch to LBL1 > jmp LBL1 LBL1: ... A LBL2: ... B ``` In (2), analyze branch simplifies this to ``` LBL0: < unknown terminator op that may branch to LBL2 > ;; jmp LBL1 <- redundant jump removed LBL1: ... A LBL2: ... B ``` In (3), MachineBlockPlacement goes ahead with its plan of putting LBL2 after the first block since that is profitable. ``` LBL0: < unknown terminator op that may branch to LBL2 > ;; jmp LBL1 <- redundant jump LBL2: ... B LBL1: ... A ``` and the program now has incorrect behavior (we no longer fall-through from `LBL0` to `LBL1`) because MBP can no longer edit LBL0. There are several possible solutions, but I went with removing the teeth off of the `analyzeBranch` calls in TailDuplicator. That makes thinking about the result of these calls easier, and breaks nothing in the lit test suite. I've also added some bookkeeping to the MachineBlockPlacement pass and used that to write an assert that would have caught this. Reviewers: chandlerc, gberry, MatzeB, iteratee Subscribers: mcrosier, llvm-commits Differential Revision: https://reviews.llvm.org/D27783 llvm-svn: 289764
- Loading branch information
Showing
3 changed files
with
116 additions
and
7 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
# RUN: llc -O3 -run-pass=block-placement -o - %s | FileCheck %s | ||
|
||
--- | | ||
; ModuleID = 'test.ll' | ||
source_filename = "test.ll" | ||
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" | ||
|
||
declare void @stub(i32*) | ||
|
||
define i32 @f(i32* %ptr, i1 %cond) { | ||
entry: | ||
br i1 %cond, label %left, label %right | ||
|
||
left: ; preds = %entry | ||
%is_null = icmp eq i32* %ptr, null | ||
br i1 %is_null, label %null, label %not_null, !prof !0, !make.implicit !1 | ||
|
||
not_null: ; preds = %left | ||
%val = load i32, i32* %ptr | ||
ret i32 %val | ||
|
||
null: ; preds = %left | ||
call void @stub(i32* %ptr) | ||
unreachable | ||
|
||
right: ; preds = %entry | ||
call void @stub(i32* null) | ||
unreachable | ||
} | ||
|
||
; Function Attrs: nounwind | ||
declare void @llvm.stackprotector(i8*, i8**) #0 | ||
|
||
attributes #0 = { nounwind } | ||
|
||
!0 = !{!"branch_weights", i32 1048575, i32 1} | ||
!1 = !{} | ||
|
||
... | ||
--- | ||
# CHECK: name: f | ||
name: f | ||
alignment: 4 | ||
tracksRegLiveness: true | ||
liveins: | ||
- { reg: '%rdi' } | ||
- { reg: '%esi' } | ||
|
||
# CHECK: %eax = FAULTING_LOAD_OP %bb.3.null, 1684, killed %rdi, 1, _, 0, _ :: (load 4 from %ir.ptr) | ||
# CHECK-NEXT: JMP_1 %bb.2.not_null | ||
# CHECK: bb.3.null: | ||
# CHECK: bb.4.right: | ||
# CHECK: bb.2.not_null: | ||
|
||
body: | | ||
bb.0.entry: | ||
successors: %bb.1.left(0x7ffff800), %bb.3.right(0x00000800) | ||
liveins: %esi, %rdi | ||
frame-setup PUSH64r undef %rax, implicit-def %rsp, implicit %rsp | ||
CFI_INSTRUCTION def_cfa_offset 16 | ||
TEST8ri %sil, 1, implicit-def %eflags, implicit killed %esi | ||
JE_1 %bb.3.right, implicit killed %eflags | ||
bb.1.left: | ||
successors: %bb.2.null(0x7ffff800), %bb.4.not_null(0x00000800) | ||
liveins: %rdi | ||
%eax = FAULTING_LOAD_OP %bb.2.null, 1684, killed %rdi, 1, _, 0, _ :: (load 4 from %ir.ptr) | ||
JMP_1 %bb.4.not_null | ||
bb.4.not_null: | ||
liveins: %rdi, %eax | ||
%rcx = POP64r implicit-def %rsp, implicit %rsp | ||
RETQ %eax | ||
bb.2.null: | ||
liveins: %rdi | ||
CALL64pcrel32 @stub, csr_64, implicit %rsp, implicit %rdi, implicit-def %rsp | ||
bb.3.right: | ||
dead %edi = XOR32rr undef %edi, undef %edi, implicit-def dead %eflags, implicit-def %rdi | ||
CALL64pcrel32 @stub, csr_64, implicit %rsp, implicit %rdi, implicit-def %rsp | ||
... |