From 4513397601a3b22d11b6c2fad000ee8e5481d694 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson@ericsson.com>
Date: Wed, 2 Nov 2016 08:55:19 +0000
Subject: [PATCH] [Reassociate] Skip analysis of dead code to avoid infinite
 loop.

Summary:
It was detected that the reassociate pass could enter an inifite
loop when analysing dead code. Simply skipping to analyse basic
blocks that are dead avoids such problems (and as a side effect
we avoid spending time on optimising dead code).

The solution is using the same Reverse Post Order ordering of the
basic blocks when doing the optimisations, as when building the
precalculated rank map. A nice side-effect of this solution is
that we now know that we only try to do optimisations for blocks
with ranked instructions.

Fixes https://llvm.org/bugs/show_bug.cgi?id=30818

Reviewers: llvm-commits, davide, eli.friedman, mehdi_amini

Subscribers: dberlin

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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@285793 91177308-0d34-0410-b5e6-96231b3b80d8
---
 include/llvm/Transforms/Scalar/Reassociate.h |  2 +-
 lib/Transforms/Scalar/Reassociate.cpp        | 17 ++++++---
 test/Transforms/Reassociate/deadcode.ll      | 37 ++++++++++++++++++++
 3 files changed, 51 insertions(+), 5 deletions(-)
 create mode 100644 test/Transforms/Reassociate/deadcode.ll

diff --git a/include/llvm/Transforms/Scalar/Reassociate.h b/include/llvm/Transforms/Scalar/Reassociate.h
index 2f56b939877..7b68b448930 100644
--- a/include/llvm/Transforms/Scalar/Reassociate.h
+++ b/include/llvm/Transforms/Scalar/Reassociate.h
@@ -65,7 +65,7 @@ public:
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &);
 
 private:
-  void BuildRankMap(Function &F);
+  void BuildRankMap(Function &F, ReversePostOrderTraversal<Function *> &RPOT);
   unsigned getRank(Value *V);
   void canonicalizeOperands(Instruction *I);
   void ReassociateExpression(BinaryOperator *I);
diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp
index ac0d7b8f1dd..e8abb5b03fb 100644
--- a/lib/Transforms/Scalar/Reassociate.cpp
+++ b/lib/Transforms/Scalar/Reassociate.cpp
@@ -145,7 +145,8 @@ static BinaryOperator *isReassociableOp(Value *V, unsigned Opcode1,
   return nullptr;
 }
 
-void ReassociatePass::BuildRankMap(Function &F) {
+void ReassociatePass::BuildRankMap(Function &F,
+                                   ReversePostOrderTraversal<Function*> &RPOT) {
   unsigned i = 2;
 
   // Assign distinct ranks to function arguments.
@@ -154,7 +155,7 @@ void ReassociatePass::BuildRankMap(Function &F) {
     DEBUG(dbgs() << "Calculated Rank[" << I->getName() << "] = " << i << "\n");
   }
 
-  ReversePostOrderTraversal<Function *> RPOT(&F);
+  // Traverse basic blocks in ReversePostOrder
   for (BasicBlock *BB : RPOT) {
     unsigned BBRank = RankMap[BB] = ++i << 16;
 
@@ -2174,11 +2175,19 @@ void ReassociatePass::ReassociateExpression(BinaryOperator *I) {
 }
 
 PreservedAnalyses ReassociatePass::run(Function &F, FunctionAnalysisManager &) {
+  // Get the functions basic blocks in Reverse Post Order. This order is used by
+  // BuildRankMap to pre calculate ranks correctly. It also excludes dead basic
+  // blocks (it has been seen that the analysis in this pass could hang when
+  // analysing dead basic blocks).
+  ReversePostOrderTraversal<Function *> RPOT(&F);
+
   // Calculate the rank map for F.
-  BuildRankMap(F);
+  BuildRankMap(F, RPOT);
 
   MadeChange = false;
-  for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI) {
+  // Traverse the same blocks that was analysed by BuildRankMap.
+  for (BasicBlock *BI : RPOT) {
+    assert(RankMap.count(&*BI) && "BB should be ranked.");
     // Optimize every instruction in the basic block.
     for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;)
       if (isInstructionTriviallyDead(&*II)) {
diff --git a/test/Transforms/Reassociate/deadcode.ll b/test/Transforms/Reassociate/deadcode.ll
new file mode 100644
index 00000000000..866cf64d2a1
--- /dev/null
+++ b/test/Transforms/Reassociate/deadcode.ll
@@ -0,0 +1,37 @@
+; RUN: opt < %s -reassociate -disable-output
+
+; It has been detected that dead loops like the one in this test case can be
+; created by -jump-threading (it was detected by a csmith generated program).
+;
+; According to -verify this is valid input (even if it could be discussed if
+; the dead loop really satisfies SSA form).
+;
+; The problem found was that the -reassociate pass ends up in an infinite loop
+; when analysing the 'deadloop1' basic block. See "Bugzilla - Bug 30818".
+define void @deadloop1() {
+  br label %endlabel
+
+deadloop1:
+  %1 = xor i32 %2, 7
+  %2 = xor i32 %1, 8
+  br label %deadloop1
+
+endlabel:
+  ret void
+}
+
+
+; Another example showing that dead code could result in infinite loops in
+; reassociate pass. See "Bugzilla - Bug 30818".
+define void @deadloop2() {
+  br label %endlabel
+
+deadloop2:
+  %1 = and i32 %2, 7
+  %2 = and i32 %3, 8
+  %3 = and i32 %1, 6
+  br label %deadloop2
+
+endlabel:
+  ret void
+}
-- 
2.40.0