[PATCH] D27783: [MachineBlockPlacement] Don't make blocks "uneditable"

Sanjoy Das via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 14 16:01:19 PST 2016

sanjoy created this revision.
sanjoy added reviewers: iteratee, chandlerc, gberry, MatzeB.
sanjoy added a subscriber: llvm-commits.
Herald added a subscriber: mcrosier.

NB: I'm new to this area, so please review with care and distrust. :)

This fixes an issue with MachineBlockPlacement due to a badly timed call
to `analyzeBranch` with `AllowModify` set to true.  The timeline is as

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:

    < unknown terminator op that may branch to LBL1 >
    jmp LBL1
    ... A
    ... B

In (2), analyze branch simplifies this to

    < unknown terminator op that may branch to LBL2 >
    ;; jmp LBL1 <- redundant jump removed
    ... A
    ... B

In (3), MachineBlockPlacement goes ahead with its plan of putting LBL2
after the first block since that is profitable.

    < unknown terminator op that may branch to LBL2 >
    ;; jmp LBL1 <- redundant jump
    ... B
    ... 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 nothing in the lit test
suite broke when I did it.

I've also added some bookkeeping to the MachineBlockPlacement pass and
used that to write an assert that would have caught this issue.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: D27783.81491.patch
Type: text/x-patch
Size: 9124 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161215/208d6c86/attachment.bin>

More information about the llvm-commits mailing list