[llvm] r338822 - [Dominators] Refine the logic of recalculate() in the DomTreeUpdater

[Chijun Sima via llvm-commits]

Author: sima
Date: Thu Aug  2 23:51:35 2018
New Revision: 338822

URL: http://llvm.org/viewvc/llvm-project?rev=338822&view=rev
Log:
[Dominators] Refine the logic of recalculate() in the DomTreeUpdater

Summary:
This patch refines the logic of `recalculate()` in the `DomTreeUpdater` in the following two aspects:
1. Previously, `recalculate()` tests whether there are pending updates/BBs awaiting deletion and then do recalculation under Lazy UpdateStrategy; and do recalculation immediately under Eager UpdateStrategy. (The former behavior is inherited from the `DeferredDominance` class). This is an inconsistency between two strategies and there is no obvious reason to do this. So the behavior is changed to always recalculate available trees when calling `recalculate()`.
2. Fix the issue of when DTU under Lazy UpdateStrategy holds nothing but with BBs awaiting deletion, after calling `recalculate()`, BBs awaiting deletion aren't flushed. An additional unittest is added to cover this case.

Reviewers: kuhar, dmgreen, brzycki, grosser, davide

Reviewed By: kuhar

Subscribers: llvm-commits

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

Modified:
    llvm/trunk/include/llvm/IR/DomTreeUpdater.h
    llvm/trunk/lib/IR/DomTreeUpdater.cpp
    llvm/trunk/unittests/IR/DomTreeUpdaterTest.cpp

Modified: llvm/trunk/include/llvm/IR/DomTreeUpdater.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DomTreeUpdater.h?rev=338822&r1=338821&r2=338822&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/DomTreeUpdater.h (original)
+++ llvm/trunk/include/llvm/IR/DomTreeUpdater.h Thu Aug  2 23:51:35 2018
@@ -159,11 +159,9 @@ public:
   void callbackDeleteBB(BasicBlock *DelBB,
                         std::function<void(BasicBlock *)> Callback);
 
-  /// Recalculate all available trees.
-  /// Under Lazy Strategy, available trees will only be recalculated if there
-  /// are pending updates or there is BasicBlock awaiting deletion. Returns true
-  /// if at least one tree is recalculated.
-  bool recalculate(Function &F);
+  /// Recalculate all available trees and flush all BasicBlocks
+  /// awaiting deletion immediately.
+  void recalculate(Function &F);
 
   /// Flush DomTree updates and return DomTree.
   /// It also flush out of date updates applied by all available trees

Modified: llvm/trunk/lib/IR/DomTreeUpdater.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DomTreeUpdater.cpp?rev=338822&r1=338821&r2=338822&view=diff
==============================================================================
--- llvm/trunk/lib/IR/DomTreeUpdater.cpp (original)
+++ llvm/trunk/lib/IR/DomTreeUpdater.cpp Thu Aug  2 23:51:35 2018
@@ -152,39 +152,34 @@ bool DomTreeUpdater::forceFlushDeletedBB
   return true;
 }
 
-bool DomTreeUpdater::recalculate(Function &F) {
-  if (!DT && !PDT)
-    return false;
+void DomTreeUpdater::recalculate(Function &F) {
 
   if (Strategy == UpdateStrategy::Eager) {
     if (DT)
       DT->recalculate(F);
     if (PDT)
       PDT->recalculate(F);
-    return true;
+    return;
   }
 
+  // There is little performance gain if we pend the recalculation under
+  // Lazy UpdateStrategy so we recalculate available trees immediately.
+
   // Prevent forceFlushDeletedBB() from erasing DomTree or PostDomTree nodes.
   IsRecalculatingDomTree = IsRecalculatingPostDomTree = true;
 
   // Because all trees are going to be up-to-date after recalculation,
   // flush awaiting deleted BasicBlocks.
-  if (forceFlushDeletedBB() || hasPendingUpdates()) {
-    if (DT)
-      DT->recalculate(F);
-    if (PDT)
-      PDT->recalculate(F);
-
-    // Resume forceFlushDeletedBB() to erase DomTree or PostDomTree nodes.
-    IsRecalculatingDomTree = IsRecalculatingPostDomTree = false;
-    PendDTUpdateIndex = PendPDTUpdateIndex = PendUpdates.size();
-    dropOutOfDateUpdates();
-    return true;
-  }
+  forceFlushDeletedBB();
+  if (DT)
+    DT->recalculate(F);
+  if (PDT)
+    PDT->recalculate(F);
 
   // Resume forceFlushDeletedBB() to erase DomTree or PostDomTree nodes.
   IsRecalculatingDomTree = IsRecalculatingPostDomTree = false;
-  return false;
+  PendDTUpdateIndex = PendPDTUpdateIndex = PendUpdates.size();
+  dropOutOfDateUpdates();
 }
 
 bool DomTreeUpdater::hasPendingUpdates() const {

Modified: llvm/trunk/unittests/IR/DomTreeUpdaterTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/DomTreeUpdaterTest.cpp?rev=338822&r1=338821&r2=338822&view=diff
==============================================================================
--- llvm/trunk/unittests/IR/DomTreeUpdaterTest.cpp (original)
+++ llvm/trunk/unittests/IR/DomTreeUpdaterTest.cpp Thu Aug  2 23:51:35 2018
@@ -699,3 +699,29 @@ TEST(DomTreeUpdater, LazyUpdateStepTest)
   DTU.flush();
   ASSERT_TRUE(DT.verify());
 }
+
+TEST(DomTreeUpdater, NoTreeTest) {
+  StringRef FuncName = "f";
+  StringRef ModuleString = R"(
+                           define i32 @f() {
+                           bb0:
+                              ret i32 0
+                           }
+                           )";
+  // Make the module.
+  LLVMContext Context;
+  std::unique_ptr<Module> M = makeLLVMModule(Context, ModuleString);
+  Function *F = M->getFunction(FuncName);
+
+  // Make the DTU.
+  DomTreeUpdater DTU(nullptr, nullptr, DomTreeUpdater::UpdateStrategy::Lazy);
+  ASSERT_FALSE(DTU.hasDomTree());
+  ASSERT_FALSE(DTU.hasPostDomTree());
+  Function::iterator FI = F->begin();
+  BasicBlock *BB0 = &*FI++;
+  // Test whether PendingDeletedBB is flushed after the recalculation.
+  DTU.deleteBB(BB0);
+  ASSERT_TRUE(DTU.hasPendingDeletedBB());
+  DTU.recalculate(*F);
+  ASSERT_FALSE(DTU.hasPendingDeletedBB());
+}