[llvm] r289764 - [MachineBlockPlacement] Don't make blocks "uneditable"
Sanjoy Das via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 14 23:28:04 PST 2016
This change also broke http://lab.llvm.org:8011/builders/clang-ppc64le-linux
I think I have addressed the issue in r289779. However if that does
not fix the issue and you're blocked on making the clang-ppc64le-linux
bot green, please don't hesitate to revert this patch. I'll revisit
this tomorrow morning (PDT).
-- Sanjoy
On Wed, Dec 14, 2016 at 9:49 PM, Sanjoy Das
<sanjoy at playingwithpointers.com> wrote:
> Hi Hal,
>
> On 12/14/16, 9:47 PM, Hal Finkel wrote:
>>
>> 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.
>
>
> That looks fine! Thanks for the fix, and sorry for the breakage!
>
> -- Sanjoy
>
>>
>> -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
>>>
>>
>
--
Sanjoy Das
http://playingwithpointers.com
More information about the llvm-commits
mailing list