llvm dev - Nov 2014 - [LLVMdev] Should the MachineVerifier accept a MBB with a single (landing pad) successor?

Hi all,

I've been investigating a machine verifier failure on the attached
testcase, and I'm tempted to say the verifier is wrong and should
accept it.  Skip the description for the proposed change.


On AArch64, the verifier complains with:

    *** Bad machine code: MBB exits via unconditional branch but
doesn't have exactly one CFG successor! ***
    - function:    t4
    - basic block: BB#5 invoke.cont41

The freshly selected relevant blocks are:

    BB#7: derived from LLVM BB %invoke.cont41
            EH_LABEL <MCSym=Ltmp4>
            B <BB#8>
        Successors according to CFG: BB#8(1) BB#9(1)

    BB#8: derived from LLVM BB %invoke.cont43
        Predecessors according to CFG: BB#7

    BB#9: derived from LLVM BB %lpad40, EH LANDING PAD
        Predecessors according to CFG: BB#7
            EH_LABEL <MCSym=Ltmp5>
            ...


The unreachable BB#8 gets removed, and we end up with:

    BB#5: derived from LLVM BB %invoke.cont41
            ...
            B <BB#8>
        Successors according to CFG: BB#8(2)
    ...
    BB#8: derived from LLVM BB %lpad40, EH LANDING PAD
        Predecessors according to CFG: BB#5


On other targets, the branch terminating BB#7 gets removed early, for
various reasons (happenstance, really).  The edge to the landing pad
is still there, but the verifier can't check anything because there's
no branch to analyze.

On AArch64, the branch remains, and the verifier hits this condition:

      if (MBB->succ_size() != 1+LandingPadSuccs.size())

So:
- the problem exists elsewhere, but is hidden by branch folder optimizations
- if the normal successor to an invoke BB is unreachable, it seems
reasonable to only have 1 successor, the landing pad.

Hence my simple change, making the verifier accept it:

--- c/lib/CodeGen/MachineVerifier.cpp
+++ w/lib/CodeGen/MachineVerifier.cpp
@@ -590,7 +590,11 @@
MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock
*MBB) {
       }
     } else if (TBB && !FBB && Cond.empty()) {
       // Block unconditionally branches somewhere.
-      if (MBB->succ_size() != 1+LandingPadSuccs.size()) {
+      // If the block has exactly one successor, that happens to be a
+      // landingpad, accept it as valid control flow.
+      if (MBB->succ_size() != 1+LandingPadSuccs.size() &&
+          (MBB->succ_size() != 1 || LandingPadSuccs.size() != 1 ||
+           *MBB->succ_begin() != *LandingPadSuccs.begin())) {
         report("MBB exits via unconditional branch but doesn't have
"
                "exactly one CFG successor!", MBB);
       } else if (!MBB->isSuccessor(TBB)) {


- Ahmed
-------------- next part --------------
A non-text attachment was scrubbed...
Name: br-simple.ll
Type: application/octet-stream
Size: 6466 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141106/a7d178a6/attachment.obj>

Ping!
- Ahmed


On Thu, Nov 6, 2014 at 11:26 AM, Ahmed Bougacha
<ahmed.bougacha at gmail.com> wrote:
> Hi all,
>
> I've been investigating a machine verifier failure on the attached
> testcase, and I'm tempted to say the verifier is wrong and should
> accept it.  Skip the description for the proposed change.
>
>
> On AArch64, the verifier complains with:
>
>     *** Bad machine code: MBB exits via unconditional branch but
> doesn't have exactly one CFG successor! ***
>     - function:    t4
>     - basic block: BB#5 invoke.cont41
>
> The freshly selected relevant blocks are:
>
>     BB#7: derived from LLVM BB %invoke.cont41
>             EH_LABEL <MCSym=Ltmp4>
>             B <BB#8>
>         Successors according to CFG: BB#8(1) BB#9(1)
>
>     BB#8: derived from LLVM BB %invoke.cont43
>         Predecessors according to CFG: BB#7
>
>     BB#9: derived from LLVM BB %lpad40, EH LANDING PAD
>         Predecessors according to CFG: BB#7
>             EH_LABEL <MCSym=Ltmp5>
>             ...
>
>
> The unreachable BB#8 gets removed, and we end up with:
>
>     BB#5: derived from LLVM BB %invoke.cont41
>             ...
>             B <BB#8>
>         Successors according to CFG: BB#8(2)
>     ...
>     BB#8: derived from LLVM BB %lpad40, EH LANDING PAD
>         Predecessors according to CFG: BB#5
>
>
> On other targets, the branch terminating BB#7 gets removed early, for
> various reasons (happenstance, really).  The edge to the landing pad
> is still there, but the verifier can't check anything because
there's
> no branch to analyze.
>
> On AArch64, the branch remains, and the verifier hits this condition:
>
>       if (MBB->succ_size() != 1+LandingPadSuccs.size())
>
> So:
> - the problem exists elsewhere, but is hidden by branch folder
optimizations
> - if the normal successor to an invoke BB is unreachable, it seems
> reasonable to only have 1 successor, the landing pad.
>
> Hence my simple change, making the verifier accept it:
>
> --- c/lib/CodeGen/MachineVerifier.cpp
> +++ w/lib/CodeGen/MachineVerifier.cpp
> @@ -590,7 +590,11 @@
> MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock
> *MBB) {
>        }
>      } else if (TBB && !FBB && Cond.empty()) {
>        // Block unconditionally branches somewhere.
> -      if (MBB->succ_size() != 1+LandingPadSuccs.size()) {
> +      // If the block has exactly one successor, that happens to be a
> +      // landingpad, accept it as valid control flow.
> +      if (MBB->succ_size() != 1+LandingPadSuccs.size() &&
> +          (MBB->succ_size() != 1 || LandingPadSuccs.size() != 1 ||
> +           *MBB->succ_begin() != *LandingPadSuccs.begin())) {
>          report("MBB exits via unconditional branch but doesn't
have "
>                 "exactly one CFG successor!", MBB);
>        } else if (!MBB->isSuccessor(TBB)) {
>
>
> - Ahmed

Hi Ahmed,

On 12 November 2014 10:47, Ahmed Bougacha <ahmed.bougacha at gmail.com>
wrote:
>> I've been investigating a machine verifier failure on the attached
>> testcase, and I'm tempted to say the verifier is wrong and should
>> accept it.  Skip the description for the proposed change.
I think you made a fair case. Each step along the way seems
reasonable, so the IR ought to be valid. I don't think we want to be
guaranteeing redundant branches will be removed, which is the only
other realistic solution.

Cheers.

Tim.