[llvm] f8a42bc - [ADCE] Fix incorrect reporting of CFG changes

[Max Kazantsev via llvm-commits]

Author: Max Kazantsev
Date: 2020-04-14T20:26:13+07:00
New Revision: f8a42bca2812fb602b0f2f24ce60f8216f6ddf87

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

LOG: [ADCE] Fix incorrect reporting of CFG changes

This patch fixes 2 related bugs in ADCE:
- `performDeadCodeElimination` does not report changes if it did ONLY
  CFG changes (affects both old and new pass managers);
- When control flow removal is enabled, new pass manager does not
  drop CFG analyses.

Both can lead to incorrect loop info after ADCE that does only CFG changes.

Differential Revision: https://reviews.llvm.org/D78103
Reviewed By: Denis Antrushin

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/ADCE.cpp
    llvm/test/Transforms/ADCE/broken-loop-info.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/ADCE.cpp b/llvm/lib/Transforms/Scalar/ADCE.cpp
index cc3d3bf7cdbf..c3709b9afffb 100644
--- a/llvm/lib/Transforms/Scalar/ADCE.cpp
+++ b/llvm/lib/Transforms/Scalar/ADCE.cpp
@@ -182,7 +182,7 @@ class AggressiveDeadCodeElimination {
 
   /// Identify connected sections of the control flow graph which have
   /// dead terminators and rewrite the control flow graph to remove them.
-  void updateDeadRegions();
+  bool updateDeadRegions();
 
   /// Set the BlockInfo::PostOrder field based on a post-order
   /// numbering of the reverse control flow graph.
@@ -505,7 +505,7 @@ void AggressiveDeadCodeElimination::markLiveBranchesFromControlDependences() {
 //===----------------------------------------------------------------------===//
 bool AggressiveDeadCodeElimination::removeDeadInstructions() {
   // Updates control and dataflow around dead blocks
-  updateDeadRegions();
+  bool RegionsUpdated = updateDeadRegions();
 
   LLVM_DEBUG({
     for (Instruction &I : instructions(F)) {
@@ -556,11 +556,11 @@ bool AggressiveDeadCodeElimination::removeDeadInstructions() {
     I->eraseFromParent();
   }
 
-  return !Worklist.empty();
+  return !Worklist.empty() || RegionsUpdated;
 }
 
 // A dead region is the set of dead blocks with a common live post-dominator.
-void AggressiveDeadCodeElimination::updateDeadRegions() {
+bool AggressiveDeadCodeElimination::updateDeadRegions() {
   LLVM_DEBUG({
     dbgs() << "final dead terminator blocks: " << '\n';
     for (auto *BB : BlocksWithDeadTerminators)
@@ -570,6 +570,7 @@ void AggressiveDeadCodeElimination::updateDeadRegions() {
 
   // Don't compute the post ordering unless we needed it.
   bool HavePostOrder = false;
+  bool Changed = false;
 
   for (auto *BB : BlocksWithDeadTerminators) {
     auto &Info = BlockInfo[BB];
@@ -624,7 +625,10 @@ void AggressiveDeadCodeElimination::updateDeadRegions() {
         .applyUpdates(DeletedEdges);
 
     NumBranchesRemoved += 1;
+    Changed = true;
   }
+
+  return Changed;
 }
 
 // reverse top-sort order
@@ -685,10 +689,14 @@ PreservedAnalyses ADCEPass::run(Function &F, FunctionAnalysisManager &FAM) {
     return PreservedAnalyses::all();
 
   PreservedAnalyses PA;
-  PA.preserveSet<CFGAnalyses>();
+  // TODO: We could track if we have actually done CFG changes.
+  if (!RemoveControlFlowFlag)
+    PA.preserveSet<CFGAnalyses>();
+  else {
+    PA.preserve<DominatorTreeAnalysis>();
+    PA.preserve<PostDominatorTreeAnalysis>();
+  }
   PA.preserve<GlobalsAA>();
-  PA.preserve<DominatorTreeAnalysis>();
-  PA.preserve<PostDominatorTreeAnalysis>();
   return PA;
 }
 

diff  --git a/llvm/test/Transforms/ADCE/broken-loop-info.ll b/llvm/test/Transforms/ADCE/broken-loop-info.ll
index d1c562c31582..8c9981f2f19c 100644
--- a/llvm/test/Transforms/ADCE/broken-loop-info.ll
+++ b/llvm/test/Transforms/ADCE/broken-loop-info.ll
@@ -1,17 +1,24 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -licm -adce -licm -S < %s | FileCheck %s
 ; RUN: opt -passes='loop(licm),adce,loop(licm)' -S < %s | FileCheck %s
 ;
-; XFAIL: *
-; REQUIRES: asserts
-;
-; This test demonstrates a bug in ADCE's work with loop info. It does some
-; changes that make loop's block unreachable, but never bothers to update
-; loop info accordingly.
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:1-p2:32:8:8:32-ni:2"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @test() {
-; CHECK-LABEL: test
+; CHECK-LABEL: @test(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    ret void
+; CHECK:       bb2:
+; CHECK-NEXT:    br label [[BB4:%.*]]
+; CHECK:       bb3:
+; CHECK-NEXT:    unreachable
+; CHECK:       bb4:
+; CHECK-NEXT:    br i1 true, label [[BB1:%.*]], label [[BB2]]
+;
 
 bb:
   br label %bb2