[llvm] r289764 - [MachineBlockPlacement] Don't make blocks "uneditable"
Hal Finkel via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 14 21:47:32 PST 2016
It was probably hard to notice with all of the things I broke, but this broke buildbots also (e.g. http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fuzzer/builds/1526/steps/build%20clang/logs/stdio). Please make sure that r289766 is a reasonable fix.
-Hal
----- Original Message -----
> From: "Sanjoy Das via llvm-commits" <llvm-commits at lists.llvm.org>
> To: llvm-commits at lists.llvm.org
> Sent: Wednesday, December 14, 2016 11:08:57 PM
> Subject: [llvm] r289764 - [MachineBlockPlacement] Don't make blocks "uneditable"
>
> Author: sanjoy
> Date: Wed Dec 14 23:08:57 2016
> New Revision: 289764
>
> URL: http://llvm.org/viewvc/llvm-project?rev=289764&view=rev
> Log:
> [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
>
> Added:
> llvm/trunk/test/CodeGen/X86/block-placement.mir
> Modified:
> llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
> llvm/trunk/lib/CodeGen/TailDuplicator.cpp
>
> Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp?rev=289764&r1=289763&r2=289764&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp Wed Dec 14
> 23:08:57 2016
> @@ -324,6 +324,14 @@ class MachineBlockPlacement : public Mac
> /// between basic blocks.
> DenseMap<MachineBasicBlock *, BlockChain *> BlockToChain;
>
> +#ifndef NDEBUG
> + /// The set of basic blocks that have terminators that cannot be
> fully
> + /// analyzed. These basic blocks cannot be re-ordered safely by
> + /// MachineBlockPlacement, and we must preserve physical layout of
> these
> + /// blocks and their successors through the pass.
> + SmallPtrSet<MachineBasicBlock *, 4> BlocksWithUnanalyzableExits;
> +#endif
> +
> /// Decrease the UnscheduledPredecessors count for all blocks in
> chain, and
> /// if the count goes to 0, add them to the appropriate work list.
> void markChainSuccessors(BlockChain &Chain, MachineBasicBlock
> *LoopHeaderBB,
> @@ -1589,6 +1597,7 @@ void MachineBlockPlacement::buildCFGChai
> << getBlockName(BB) << " -> " <<
> getBlockName(NextBB)
> << "\n");
> Chain->merge(NextBB, nullptr);
> + BlocksWithUnanalyzableExits.insert(&*BB);
> FI = NextFI;
> BB = NextBB;
> }
> @@ -1661,6 +1670,19 @@ void MachineBlockPlacement::buildCFGChai
> Cond.clear();
> MachineBasicBlock *TBB = nullptr, *FBB = nullptr; // For
> AnalyzeBranch.
>
> +#ifndef NDEBUG
> + if (!BlocksWithUnanalyzableExits.count(PrevBB)) {
> + // Given the exact block placement we chose, we may actually
> not _need_ to
> + // be able to edit PrevBB's terminator sequence, but not being
> _able_ to
> + // do that at this point is a bug.
> + assert((!TII->analyzeBranch(*PrevBB, TBB, FBB, Cond) ||
> + !PrevBB->canFallThrough()) &&
> + "Unexpected block with un-analyzable fallthrough!");
> + Cond.clear();
> + TBB = FBB = nullptr;
> + }
> +#endif
> +
> // The "PrevBB" is not yet updated to reflect current code
> layout, so,
> // o. it may fall-through to a block without explicit "goto"
> instruction
> // before layout, and no longer fall-through it after
> layout; or
>
> Modified: llvm/trunk/lib/CodeGen/TailDuplicator.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/TailDuplicator.cpp?rev=289764&r1=289763&r2=289764&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/TailDuplicator.cpp (original)
> +++ llvm/trunk/lib/CodeGen/TailDuplicator.cpp Wed Dec 14 23:08:57
> 2016
> @@ -550,8 +550,8 @@ bool TailDuplicator::shouldTailDuplicate
> // blocks with unanalyzable fallthrough get layed out
> contiguously.
> MachineBasicBlock *PredTBB = nullptr, *PredFBB = nullptr;
> SmallVector<MachineOperand, 4> PredCond;
> - if (TII->analyzeBranch(TailBB, PredTBB, PredFBB, PredCond, true)
> - && TailBB.canFallThrough())
> + if (TII->analyzeBranch(TailBB, PredTBB, PredFBB, PredCond) &&
> + TailBB.canFallThrough())
> return false;
>
> // If the target has hardware branch prediction that can handle
> indirect
> @@ -660,7 +660,7 @@ bool TailDuplicator::canCompletelyDuplic
>
> MachineBasicBlock *PredTBB = nullptr, *PredFBB = nullptr;
> SmallVector<MachineOperand, 4> PredCond;
> - if (TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond,
> true))
> + if (TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond))
> return false;
>
> if (!PredCond.empty())
> @@ -687,7 +687,7 @@ bool TailDuplicator::duplicateSimpleBB(
>
> MachineBasicBlock *PredTBB = nullptr, *PredFBB = nullptr;
> SmallVector<MachineOperand, 4> PredCond;
> - if (TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond,
> true))
> + if (TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond))
> continue;
>
> Changed = true;
> @@ -750,7 +750,7 @@ bool TailDuplicator::canTailDuplicate(Ma
>
> MachineBasicBlock *PredTBB, *PredFBB;
> SmallVector<MachineOperand, 4> PredCond;
> - if (TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond, true))
> + if (TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond))
> return false;
> if (!PredCond.empty())
> return false;
> @@ -833,7 +833,7 @@ bool TailDuplicator::tailDuplicate(bool
> // Simplify
> MachineBasicBlock *PredTBB, *PredFBB;
> SmallVector<MachineOperand, 4> PredCond;
> - TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond, true);
> + TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond);
>
> NumTailDupAdded += TailBB->size() - 1; // subtract one for
> removed branch
>
> @@ -861,7 +861,7 @@ bool TailDuplicator::tailDuplicate(bool
> if (PrevBB->succ_size() == 1 &&
> // Layout preds are not always CFG preds. Check.
> *PrevBB->succ_begin() == TailBB &&
> - !TII->analyzeBranch(*PrevBB, PriorTBB, PriorFBB, PriorCond,
> true) &&
> + !TII->analyzeBranch(*PrevBB, PriorTBB, PriorFBB, PriorCond) &&
> PriorCond.empty() &&
> (!PriorTBB || PriorTBB == TailBB) &&
> TailBB->pred_size() == 1 &&
>
> Added: llvm/trunk/test/CodeGen/X86/block-placement.mir
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/block-placement.mir?rev=289764&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/block-placement.mir (added)
> +++ llvm/trunk/test/CodeGen/X86/block-placement.mir Wed Dec 14
> 23:08:57 2016
> @@ -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
> +
> +...
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list