[llvm] r313696 - [MIRPrinter] Print empty successor lists when they cannot be guessed

[Quentin Colombet via llvm-commits]

Author: qcolombet
Date: Tue Sep 19 16:34:12 2017
New Revision: 313696

URL: http://llvm.org/viewvc/llvm-project?rev=313696&view=rev
Log:
[MIRPrinter] Print empty successor lists when they cannot be guessed

This re-applies commit r313685, this time with the proper updates to
the test cases.

Original commit message:
Unreachable blocks in the machine instr representation are these
weird empty blocks with no successors.
The MIR printer used to not print empty lists of successors. However,
the MIR parser now treats non-printed list of successors as "please
guess it for me". As a result, the parser tries to guess the list of
successors and given the block is empty, just assumes it falls through
the next block (if any).

For instance, the following test case used to fail the verifier.
The MIR printer would print

         entry
        /      \
   true (def)   false (no list of successors)
       |
 split.true (use)

The MIR parser would understand this:

         entry
        /      \
   true (def)   false
       |        /  <-- invalid edge
 split.true (use)

Because of the invalid edge, we get the "def does not
dominate all uses" error.

The fix consists in printing empty successor lists, so that the parser
knows what to do for unreachable blocks.

rdar://problem/34022159

Added:
    llvm/trunk/test/CodeGen/MIR/X86/unreachable_block.ll
Modified:
    llvm/trunk/lib/CodeGen/MIRPrinter.cpp
    llvm/trunk/test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir

Modified: llvm/trunk/lib/CodeGen/MIRPrinter.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MIRPrinter.cpp?rev=313696&r1=313695&r2=313696&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/MIRPrinter.cpp (original)
+++ llvm/trunk/lib/CodeGen/MIRPrinter.cpp Tue Sep 19 16:34:12 2017
@@ -598,8 +598,14 @@ void MIPrinter::print(const MachineBasic
   bool HasLineAttributes = false;
   // Print the successors
   bool canPredictProbs = canPredictBranchProbabilities(MBB);
-  if (!MBB.succ_empty() && (!SimplifyMIR || !canPredictProbs ||
-                            !canPredictSuccessors(MBB))) {
+  // Even if the list of successors is empty, if we cannot guess it,
+  // we need to print it to tell the parser that the list is empty.
+  // This is needed, because MI model unreachable as empty blocks
+  // with an empty successor list. If the parser would see that
+  // without the successor list, it would guess the code would
+  // fallthrough.
+  if ((!MBB.succ_empty() && !SimplifyMIR) || !canPredictProbs ||
+      !canPredictSuccessors(MBB)) {
     OS.indent(2) << "successors: ";
     for (auto I = MBB.succ_begin(), E = MBB.succ_end(); I != E; ++I) {
       if (I != MBB.succ_begin())

Modified: llvm/trunk/test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir?rev=313696&r1=313695&r2=313696&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir (original)
+++ llvm/trunk/test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir Tue Sep 19 16:34:12 2017
@@ -52,8 +52,9 @@ body:             |
 #CHECK:     B %bb.3
 
 # Empty bb.2, originally containing "unreachable" and thus has no successors
+# and we cannot guess them: we should print an empty list of successors.
 #CHECK:   bb.2:
-#CHECK-NOT: successors
+#CHECK: successors:{{ *$}}
 
 #CHECK:   bb.3:
 #CHECK:     successors: %bb.1

Added: llvm/trunk/test/CodeGen/MIR/X86/unreachable_block.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/MIR/X86/unreachable_block.ll?rev=313696&view=auto
==============================================================================
--- llvm/trunk/test/CodeGen/MIR/X86/unreachable_block.ll (added)
+++ llvm/trunk/test/CodeGen/MIR/X86/unreachable_block.ll Tue Sep 19 16:34:12 2017
@@ -0,0 +1,48 @@
+; RUN: llc -mtriple x86_64-- -stop-before peephole-opt -o %t.mir %s
+; RUN: llc -mtriple x86_64-- -run-pass none %t.mir -verify-machineinstrs -o - | FileCheck %s
+
+; Unreachable blocks in the machine instr representation are these
+; weird empty blocks with no successors.
+; The MIR printer used to not print empty lists of successors. However,
+; the MIR parser now treats non-printed list of successors as "please
+; guess it for me". As a result, the parser tries to guess the list of
+; successors and given the block is empty, just assumes it falls through
+; the next block.
+;
+; The following test case used to fail the verifier because the false
+; path ended up falling through split.true and now, the definition of
+; %v does not dominate all its uses.
+; Indeed, we go from the following CFG:
+;          entry
+;         /      \
+;    true (def)   false
+;        |
+;  split.true (use)
+;
+; To this one:
+;          entry
+;         /      \
+;    true (def)   false
+;        |        /  <-- invalid edge
+;  split.true (use)
+;
+; Because of the invalid edge, we get the "def does not
+; dominate all uses" error.
+;
+; CHECK-LABEL: name: foo
+; CHECK-LABEL: bb.{{[0-9]+}}.false:
+; CHECK-NEXT: successors:
+; CHECK-NOT: %bb.{{[0-9]+}}.split.true
+; CHECK-LABEL: bb.{{[0-9]+}}.split.true:
+define void @foo(i32* %bar) {
+  br i1 undef, label %true, label %false
+true:
+  %v = load i32, i32* %bar
+  br label %split.true
+false:
+  unreachable
+split.true:
+  %vInc = add i32 %v, 1
+  store i32 %vInc, i32* %bar
+  ret void
+}