From 5b32f60ec31ce136edac6f693538aeb6039f4ad0 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Tue, 28 May 2019 21:28:24 +0000 Subject: [PATCH] Revert "[CorrelatedValuePropagation] Fix prof branch_weights metadata handling for SwitchInst" This reverts commit 53f2f3286572cb879b3861d7c15480e4d830dd3b. As reported on D62126, this causes assertion failures if the switch has incorrect branch_weights metadata, which may happen as a result of other transforms not handling it correctly yet. llvm-svn: 361881 --- .../Scalar/CorrelatedValuePropagation.cpp | 123 +++++++++--------- .../CorrelatedValuePropagation/profmd.ll | 119 ----------------- 2 files changed, 59 insertions(+), 183 deletions(-) delete mode 100644 llvm/test/Transforms/CorrelatedValuePropagation/profmd.ll diff --git a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp index 4cb4d21754a1..4e4715be61ae 100644 --- a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp +++ b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp @@ -306,11 +306,11 @@ static bool processCmp(CmpInst *Cmp, LazyValueInfo *LVI) { /// that cannot fire no matter what the incoming edge can safely be removed. If /// a case fires on every incoming edge then the entire switch can be removed /// and replaced with a branch to the case destination. -static bool processSwitch(SwitchInst *I, LazyValueInfo *LVI, +static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI, DominatorTree *DT) { DomTreeUpdater DTU(*DT, DomTreeUpdater::UpdateStrategy::Lazy); - Value *Cond = I->getCondition(); - BasicBlock *BB = I->getParent(); + Value *Cond = SI->getCondition(); + BasicBlock *BB = SI->getParent(); // If the condition was defined in same block as the switch then LazyValueInfo // currently won't say anything useful about it, though in theory it could. @@ -327,72 +327,67 @@ static bool processSwitch(SwitchInst *I, LazyValueInfo *LVI, for (auto *Succ : successors(BB)) SuccessorsCount[Succ]++; - { // Scope for SwitchInstProfUpdateWrapper. It must not live during - // ConstantFoldTerminator() as the underlying SwitchInst can be changed. - SwitchInstProfUpdateWrapper SI(*I); + for (auto CI = SI->case_begin(), CE = SI->case_end(); CI != CE;) { + ConstantInt *Case = CI->getCaseValue(); - for (auto CI = SI->case_begin(), CE = SI->case_end(); CI != CE;) { - ConstantInt *Case = CI->getCaseValue(); - - // Check to see if the switch condition is equal to/not equal to the case - // value on every incoming edge, equal/not equal being the same each time. - LazyValueInfo::Tristate State = LazyValueInfo::Unknown; - for (pred_iterator PI = PB; PI != PE; ++PI) { - // Is the switch condition equal to the case value? - LazyValueInfo::Tristate Value = LVI->getPredicateOnEdge(CmpInst::ICMP_EQ, - Cond, Case, *PI, - BB, SI); - // Give up on this case if nothing is known. - if (Value == LazyValueInfo::Unknown) { - State = LazyValueInfo::Unknown; - break; - } - - // If this was the first edge to be visited, record that all other edges - // need to give the same result. - if (PI == PB) { - State = Value; - continue; - } - - // If this case is known to fire for some edges and known not to fire for - // others then there is nothing we can do - give up. - if (Value != State) { - State = LazyValueInfo::Unknown; - break; - } - } - - if (State == LazyValueInfo::False) { - // This case never fires - remove it. - BasicBlock *Succ = CI->getCaseSuccessor(); - Succ->removePredecessor(BB); - CI = SI.removeCase(CI); - CE = SI->case_end(); - - // The condition can be modified by removePredecessor's PHI simplification - // logic. - Cond = SI->getCondition(); - - ++NumDeadCases; - Changed = true; - if (--SuccessorsCount[Succ] == 0) - DTU.applyUpdatesPermissive({{DominatorTree::Delete, BB, Succ}}); - continue; - } - if (State == LazyValueInfo::True) { - // This case always fires. Arrange for the switch to be turned into an - // unconditional branch by replacing the switch condition with the case - // value. - SI->setCondition(Case); - NumDeadCases += SI->getNumCases(); - Changed = true; + // Check to see if the switch condition is equal to/not equal to the case + // value on every incoming edge, equal/not equal being the same each time. + LazyValueInfo::Tristate State = LazyValueInfo::Unknown; + for (pred_iterator PI = PB; PI != PE; ++PI) { + // Is the switch condition equal to the case value? + LazyValueInfo::Tristate Value = LVI->getPredicateOnEdge(CmpInst::ICMP_EQ, + Cond, Case, *PI, + BB, SI); + // Give up on this case if nothing is known. + if (Value == LazyValueInfo::Unknown) { + State = LazyValueInfo::Unknown; break; } - // Increment the case iterator since we didn't delete it. - ++CI; + // If this was the first edge to be visited, record that all other edges + // need to give the same result. + if (PI == PB) { + State = Value; + continue; + } + + // If this case is known to fire for some edges and known not to fire for + // others then there is nothing we can do - give up. + if (Value != State) { + State = LazyValueInfo::Unknown; + break; + } } + + if (State == LazyValueInfo::False) { + // This case never fires - remove it. + BasicBlock *Succ = CI->getCaseSuccessor(); + Succ->removePredecessor(BB); + CI = SI->removeCase(CI); + CE = SI->case_end(); + + // The condition can be modified by removePredecessor's PHI simplification + // logic. + Cond = SI->getCondition(); + + ++NumDeadCases; + Changed = true; + if (--SuccessorsCount[Succ] == 0) + DTU.applyUpdatesPermissive({{DominatorTree::Delete, BB, Succ}}); + continue; + } + if (State == LazyValueInfo::True) { + // This case always fires. Arrange for the switch to be turned into an + // unconditional branch by replacing the switch condition with the case + // value. + SI->setCondition(Case); + NumDeadCases += SI->getNumCases(); + Changed = true; + break; + } + + // Increment the case iterator since we didn't delete it. + ++CI; } if (Changed) diff --git a/llvm/test/Transforms/CorrelatedValuePropagation/profmd.ll b/llvm/test/Transforms/CorrelatedValuePropagation/profmd.ll deleted file mode 100644 index 493b4c2273e2..000000000000 --- a/llvm/test/Transforms/CorrelatedValuePropagation/profmd.ll +++ /dev/null @@ -1,119 +0,0 @@ -; RUN: opt < %s -correlated-propagation -S | FileCheck %s - -; Removed several cases from switch. -define i32 @switch1(i32 %s) { -; CHECK-LABEL: @switch1( -; CHECK-NEXT: entry: -; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[S:%.*]], 0 -; CHECK-NEXT: br i1 [[CMP]], label [[NEGATIVE:%.*]], label [[OUT:%.*]] -; -entry: - %cmp = icmp slt i32 %s, 0 - br i1 %cmp, label %negative, label %out - -negative: -; CHECK: negative: -; CHECK-NEXT: switch i32 [[S]], label [[OUT]] [ -; CHECK-NEXT: i32 -2, label [[NEXT:%.*]] -; CHECK-NEXT: i32 -1, label [[NEXT]] - switch i32 %s, label %out [ - i32 0, label %out - i32 1, label %out - i32 -1, label %next - i32 -2, label %next - i32 2, label %out - i32 3, label %out -; CHECK-NEXT: !prof ![[MD0:[0-9]+]] - ], !prof !{!"branch_weights", i32 99, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6} - -out: - %p = phi i32 [ 1, %entry ], [ -1, %negative ], [ -1, %negative ], [ -1, %negative ], [ -1, %negative ], [ -1, %negative ] - ret i32 %p - -next: - %q = phi i32 [ 0, %negative ], [ 0, %negative ] - ret i32 %q -} - -; Removed all cases from switch. -define i32 @switch2(i32 %s) { -; CHECK-LABEL: @switch2( -; CHECK-NEXT: entry: -; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 [[S:%.*]], 0 -; CHECK-NEXT: br i1 [[CMP]], label [[POSITIVE:%.*]], label [[OUT:%.*]] -; -entry: - %cmp = icmp sgt i32 %s, 0 - br i1 %cmp, label %positive, label %out - -positive: - switch i32 %s, label %out [ - i32 0, label %out - i32 -1, label %next - i32 -2, label %next - ], !prof !{!"branch_weights", i32 99, i32 1, i32 2, i32 3} - -out: - %p = phi i32 [ -1, %entry ], [ 1, %positive ], [ 1, %positive ] - ret i32 %p - -next: - %q = phi i32 [ 0, %positive ], [ 0, %positive ] - ret i32 %q -} - -; Change switch into conditional branch. -define i32 @switch3(i32 %s) { -; CHECK-LABEL: @switch3( -; -entry: - %cmp = icmp sgt i32 %s, 0 - br i1 %cmp, label %positive, label %out - -positive: -; CHECK: positive: -; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 %s, 1 -; CHECK-NEXT: br i1 [[CMP]], label [[NEXT:%.*]], label [[OUT:%.*]], !prof ![[MD1:[0-9]+]] - switch i32 %s, label %out [ - i32 1, label %next - i32 -1, label %next - i32 -2, label %next - ], !prof !{!"branch_weights", i32 99, i32 1, i32 2, i32 3} - -out: - %p = phi i32 [ -1, %entry ], [ 1, %positive ] - ret i32 %p - -next: - %q = phi i32 [ 0, %positive ], [ 0, %positive ], [ 0, %positive ] - ret i32 %q -} - -; Removed all cases from switch. -define i32 @switch4(i32 %s) { -; CHECK-LABEL: @switch4( -; CHECK-NEXT: entry: -; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[S:%.*]], 0 -; CHECK-NEXT: br i1 [[CMP]], label [[NEGATIVE:%.*]], label [[OUT:%.*]] -; -entry: - %cmp = icmp slt i32 %s, 0 - br i1 %cmp, label %negative, label %out - -negative: -; CHECK: negative: -; CHECK-NEXT: br label %out - switch i32 %s, label %out [ - i32 0, label %out - i32 1, label %out - i32 2, label %out - i32 3, label %out - ], !prof !{!"branch_weights", i32 99, i32 1, i32 2, i32 3, i32 4} - -out: - %p = phi i32 [ 1, %entry ], [ -1, %negative ], [ -1, %negative ], [ -1, %negative ], [ -1, %negative ], [ -1, %negative ] - ret i32 %p -} - -; CHECK: ![[MD0]] = !{!"branch_weights", i32 99, i32 4, i32 3} -; CHECK: ![[MD1]] = !{!"branch_weights", i32 1, i32 99}