[llvm] d5dc396 - [DFAJumpThreading] Determinator BB should precede switch-defining BB

[Alex Zhikhartsev via llvm-commits]

Author: Alexey Zhikhartsev
Date: 2021-12-24T10:27:03-05:00
New Revision: d5dc3964a7417a34fe581d93ff9642923f8a634d

URL: https://github.com/llvm/llvm-project/commit/d5dc3964a7417a34fe581d93ff9642923f8a634d
DIFF: https://github.com/llvm/llvm-project/commit/d5dc3964a7417a34fe581d93ff9642923f8a634d.diff

LOG: [DFAJumpThreading] Determinator BB should precede switch-defining BB

Otherwise, it is possible that the state defined in the determinator
block defines the state for the next iteration of the loop, rather than
for the current one.

Fixes llvm-test-suite's
SingleSource/Regression/C/gcc-c-torture/execute/pr80421.c

Differential Revision: https://reviews.llvm.org/D115832

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
    llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
index 8c45232060706..dda1a2f080768 100644
--- a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
@@ -588,7 +588,7 @@ struct AllSwitchPaths {
         PrevBB = BB;
       }
 
-      if (TPath.isExitValueSet())
+      if (TPath.isExitValueSet() && isSupported(TPath))
         TPaths.push_back(TPath);
     }
   }
@@ -683,6 +683,62 @@ struct AllSwitchPaths {
     return Res;
   }
 
+  /// The determinator BB should precede the switch-defining BB.
+  ///
+  /// Otherwise, it is possible that the state defined in the determinator block
+  /// defines the state for the next iteration of the loop, rather than for the
+  /// current one.
+  ///
+  /// Currently supported paths:
+  /// \code
+  /// < switch bb1 determ def > [ 42, determ ]
+  /// < switch_and_def bb1 determ > [ 42, determ ]
+  /// < switch_and_def_and_determ bb1 > [ 42, switch_and_def_and_determ ]
+  /// \endcode
+  ///
+  /// Unsupported paths:
+  /// \code
+  /// < switch bb1 def determ > [ 43, determ ]
+  /// < switch_and_determ bb1 def > [ 43, switch_and_determ ]
+  /// \endcode
+  bool isSupported(const ThreadingPath &TPath) {
+    Instruction *SwitchCondI = dyn_cast<Instruction>(Switch->getCondition());
+    assert(SwitchCondI);
+    if (!SwitchCondI)
+      return false;
+
+    const BasicBlock *SwitchCondDefBB = SwitchCondI->getParent();
+    const BasicBlock *SwitchCondUseBB = Switch->getParent();
+    const BasicBlock *DeterminatorBB = TPath.getDeterminatorBB();
+
+    assert(
+        SwitchCondUseBB == TPath.getPath().front() &&
+        "The first BB in a threading path should have the switch instruction");
+    if (SwitchCondUseBB != TPath.getPath().front())
+      return false;
+
+    // Make DeterminatorBB the first element in Path.
+    PathType Path = TPath.getPath();
+    auto ItDet = std::find(Path.begin(), Path.end(), DeterminatorBB);
+    std::rotate(Path.begin(), ItDet, Path.end());
+
+    bool IsDetBBSeen = false;
+    bool IsDefBBSeen = false;
+    bool IsUseBBSeen = false;
+    for (BasicBlock *BB : Path) {
+      if (BB == DeterminatorBB)
+        IsDetBBSeen = true;
+      if (BB == SwitchCondDefBB)
+        IsDefBBSeen = true;
+      if (BB == SwitchCondUseBB)
+        IsUseBBSeen = true;
+      if (IsDetBBSeen && IsUseBBSeen && !IsDefBBSeen)
+        return false;
+    }
+
+    return true;
+  }
+
   SwitchInst *Switch;
   BasicBlock *SwitchBlock;
   OptimizationRemarkEmitter *ORE;

diff  --git a/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll b/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll
index 7dfa1fcc3a779..ccc9b38759c41 100644
--- a/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll
+++ b/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll
@@ -109,10 +109,16 @@ exit:
 
 declare void @baz()
 
-; Verify that having the switch block as a determinator is handled correctly.
-define i32 @main() {
-; CHECK: < bb43 bb59 bb3 bb31 bb41 > [ 77, bb43 ]
-; CHECK-NEXT: < bb43 bb49 bb59 bb3 bb31 bb41 > [ 77, bb43 ]
+; Do not jump-thread those paths where the determinator basic block does not
+; precede the basic block that defines the switch condition.
+;
+; Otherwise, it is possible that the state defined in the determinator block
+; defines the state for the next iteration of the loop, rather than for the
+; current one.
+define i32 @wrong_bb_order() {
+; CHECK-LABEL: DFA Jump threading: wrong_bb_order
+; CHECK-NOT: < bb43 bb59 bb3 bb31 bb41 > [ 77, bb43 ]
+; CHECK-NOT: < bb43 bb49 bb59 bb3 bb31 bb41 > [ 77, bb43 ]
 bb:
   %i = alloca [420 x i8], align 1
   %i2 = getelementptr inbounds [420 x i8], [420 x i8]* %i, i64 0, i64 390