From 95f7f7c21b47f1739e4a901d428af970b7d7c0b9 Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Mon, 22 Mar 2021 17:45:32 -0400 Subject: [PATCH] Revert "[SimplifyCFG] use profile metadata to refine merging branch conditions" This reverts commit 27ae17a6b0145a559c501c35ded0ab4e9dd69e8e. There are bot failures that end with: #4 0x00007fff7ae3c9b8 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0 #5 0x00007fff84e504d8 (linux-vdso64.so.1+0x4d8) #6 0x00007fff7c419a5c llvm::TargetTransformInfo::getPredictableBranchThreshold() const (/home/buildbots/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1.install/bin/../lib/libLLVMAnalysis.so.13git+0x479a5c) ...but not sure how to trigger that yet. --- llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 59 ++++++------------ llvm/test/Transforms/PGOProfile/chr.ll | 14 ++--- .../SimplifyCFG/preserve-branchweights.ll | 60 ++++++------------- 3 files changed, 44 insertions(+), 89 deletions(-) diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index e3606cf3dd6a..1f9fa611a9b2 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -63,7 +63,6 @@ #include "llvm/IR/User.h" #include "llvm/IR/Value.h" #include "llvm/IR/ValueHandle.h" -#include "llvm/Support/BranchProbability.h" #include "llvm/Support/Casting.h" #include "llvm/Support/CommandLine.h" #include "llvm/Support/Debug.h" @@ -2841,52 +2840,30 @@ static bool extractPredSuccWeights(BranchInst *PBI, BranchInst *BI, } } -/// Determine if the two branches share a common destination and deduce a glue -/// that joins branch's conditions to arrive at the common destination if that -/// would be profitable. +// Determine if the two branches share a common destination, +// and deduce a glue that we need to use to join branch's conditions +// to arrive at the common destination. static Optional> -shouldFoldCondBranchesToCommonDestination(BranchInst *BI, BranchInst *PBI, - const TargetTransformInfo *TTI) { +CheckIfCondBranchesShareCommonDestination(BranchInst *BI, BranchInst *PBI) { assert(BI && PBI && BI->isConditional() && PBI->isConditional() && "Both blocks must end with a conditional branches."); assert(is_contained(predecessors(BI->getParent()), PBI->getParent()) && "PredBB must be a predecessor of BB."); - // We have the potential to fold the conditions together, but if the - // predecessor branch is predictable, we may not want to merge them. - uint64_t PTWeight, PFWeight; - BranchProbability PBITrueProb, Likely; - if (PBI->extractProfMetadata(PTWeight, PFWeight) && - (PTWeight + PFWeight) != 0) { - PBITrueProb = - BranchProbability::getBranchProbability(PTWeight, PTWeight + PFWeight); - Likely = TTI->getPredictableBranchThreshold(); - } - - if (PBI->getSuccessor(0) == BI->getSuccessor(0)) { - // Speculate the 2nd condition unless the 1st is probably true. - if (PBITrueProb.isUnknown() || PBITrueProb < Likely) - return {{Instruction::Or, false}}; - } else if (PBI->getSuccessor(1) == BI->getSuccessor(1)) { - // Speculate the 2nd condition unless the 1st is probably false. - if (PBITrueProb.isUnknown() || PBITrueProb.getCompl() < Likely) - return {{Instruction::And, false}}; - } else if (PBI->getSuccessor(0) == BI->getSuccessor(1)) { - // Speculate the 2nd condition unless the 1st is probably true. - if (PBITrueProb.isUnknown() || PBITrueProb < Likely) - return {{Instruction::And, true}}; - } else if (PBI->getSuccessor(1) == BI->getSuccessor(0)) { - // Speculate the 2nd condition unless the 1st is probably false. - if (PBITrueProb.isUnknown() || PBITrueProb.getCompl() < Likely) - return {{Instruction::Or, true}}; - } + if (PBI->getSuccessor(0) == BI->getSuccessor(0)) + return {{Instruction::Or, false}}; + else if (PBI->getSuccessor(1) == BI->getSuccessor(1)) + return {{Instruction::And, false}}; + else if (PBI->getSuccessor(0) == BI->getSuccessor(1)) + return {{Instruction::And, true}}; + else if (PBI->getSuccessor(1) == BI->getSuccessor(0)) + return {{Instruction::Or, true}}; return None; } -static bool performBranchToCommonDestFolding(BranchInst *BI, BranchInst *PBI, +static bool PerformBranchToCommonDestFolding(BranchInst *BI, BranchInst *PBI, DomTreeUpdater *DTU, - MemorySSAUpdater *MSSAU, - const TargetTransformInfo *TTI) { + MemorySSAUpdater *MSSAU) { BasicBlock *BB = BI->getParent(); BasicBlock *PredBlock = PBI->getParent(); @@ -2894,7 +2871,7 @@ static bool performBranchToCommonDestFolding(BranchInst *BI, BranchInst *PBI, Instruction::BinaryOps Opc; bool InvertPredCond; std::tie(Opc, InvertPredCond) = - *shouldFoldCondBranchesToCommonDestination(BI, PBI, TTI); + *CheckIfCondBranchesShareCommonDestination(BI, PBI); LLVM_DEBUG(dbgs() << "FOLDING BRANCH TO COMMON DEST:\n" << *PBI << *BB); @@ -3082,8 +3059,8 @@ bool llvm::FoldBranchToCommonDest(BranchInst *BI, DomTreeUpdater *DTU, // Determine if the two branches share a common destination. Instruction::BinaryOps Opc; bool InvertPredCond; - if (auto Recipe = shouldFoldCondBranchesToCommonDestination(BI, PBI, TTI)) - std::tie(Opc, InvertPredCond) = *Recipe; + if (auto Recepie = CheckIfCondBranchesShareCommonDestination(BI, PBI)) + std::tie(Opc, InvertPredCond) = *Recepie; else continue; @@ -3100,7 +3077,7 @@ bool llvm::FoldBranchToCommonDest(BranchInst *BI, DomTreeUpdater *DTU, continue; } - return performBranchToCommonDestFolding(BI, PBI, DTU, MSSAU, TTI); + return PerformBranchToCommonDestFolding(BI, PBI, DTU, MSSAU); } return Changed; } diff --git a/llvm/test/Transforms/PGOProfile/chr.ll b/llvm/test/Transforms/PGOProfile/chr.ll index ddf4811a0363..8a05a624209e 100644 --- a/llvm/test/Transforms/PGOProfile/chr.ll +++ b/llvm/test/Transforms/PGOProfile/chr.ll @@ -1277,12 +1277,11 @@ define i32 @test_chr_14(i32* %i, i32* %j, i32 %sum0, i1 %pred, i32 %z) !prof !14 ; CHECK-LABEL: @test_chr_14( ; CHECK-NEXT: entry: ; CHECK-NEXT: [[I0:%.*]] = load i32, i32* [[I:%.*]], align 4 -; CHECK-NEXT: [[V1:%.*]] = icmp eq i32 [[Z:%.*]], 1 -; CHECK-NEXT: br i1 [[V1]], label [[BB1:%.*]], label [[ENTRY_SPLIT_NONCHR:%.*]], !prof !15 -; CHECK: entry.split.nonchr: +; CHECK-NEXT: [[V1:%.*]] = icmp ne i32 [[Z:%.*]], 1 ; CHECK-NEXT: [[V0:%.*]] = icmp eq i32 [[Z]], 0 ; CHECK-NEXT: [[V3_NONCHR:%.*]] = and i1 [[V0]], [[PRED:%.*]] -; CHECK-NEXT: br i1 [[V3_NONCHR]], label [[BB0_NONCHR:%.*]], label [[BB1]], !prof !16 +; CHECK-NEXT: [[OR_COND:%.*]] = and i1 [[V1]], [[V3_NONCHR]] +; CHECK-NEXT: br i1 [[OR_COND]], label [[BB0_NONCHR:%.*]], label [[BB1:%.*]], !prof !19 ; CHECK: bb0.nonchr: ; CHECK-NEXT: call void @foo() ; CHECK-NEXT: br label [[BB1]] @@ -1913,7 +1912,7 @@ define i32 @test_chr_21(i64 %i, i64 %k, i64 %j) !prof !14 { ; CHECK-NEXT: switch i64 [[I]], label [[BB2:%.*]] [ ; CHECK-NEXT: i64 2, label [[BB3_NONCHR2:%.*]] ; CHECK-NEXT: i64 86, label [[BB2_NONCHR1:%.*]] -; CHECK-NEXT: ], !prof !19 +; CHECK-NEXT: ], !prof !20 ; CHECK: bb2: ; CHECK-NEXT: call void @foo() ; CHECK-NEXT: call void @foo() @@ -2490,14 +2489,14 @@ define void @test_chr_24(i32* %i) !prof !14 { ; CHECK-NEXT: [[TMP0:%.*]] = load i32, i32* [[I:%.*]], align 4 ; CHECK-NEXT: [[TMP1:%.*]] = and i32 [[TMP0]], 1 ; CHECK-NEXT: [[TMP2:%.*]] = icmp eq i32 [[TMP1]], 0 -; CHECK-NEXT: br i1 [[TMP2]], label [[BB1:%.*]], label [[BB0:%.*]], !prof !20 +; CHECK-NEXT: br i1 [[TMP2]], label [[BB1:%.*]], label [[BB0:%.*]], !prof !21 ; CHECK: bb0: ; CHECK-NEXT: call void @foo() ; CHECK-NEXT: br label [[BB1]] ; CHECK: bb1: ; CHECK-NEXT: [[TMP3:%.*]] = and i32 [[TMP0]], 2 ; CHECK-NEXT: [[TMP4:%.*]] = icmp eq i32 [[TMP3]], 0 -; CHECK-NEXT: br i1 [[TMP4]], label [[BB3:%.*]], label [[BB2:%.*]], !prof !20 +; CHECK-NEXT: br i1 [[TMP4]], label [[BB3:%.*]], label [[BB2:%.*]], !prof !21 ; CHECK: bb2: ; CHECK-NEXT: call void @foo() ; CHECK-NEXT: br label [[BB3]] @@ -2551,3 +2550,4 @@ bb3: ; CHECK: !16 = !{!"branch_weights", i32 0, i32 1} ; CHECK: !17 = !{!"branch_weights", i32 1, i32 1} ; CHECK: !18 = !{!"branch_weights", i32 1, i32 0} +; CHECK: !19 = !{!"branch_weights", i32 0, i32 1000} diff --git a/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll b/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll index ddf28d242644..1e966c2f4c4a 100644 --- a/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll +++ b/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll @@ -636,17 +636,16 @@ exit: ret i32 %outval } -; Merging the icmps with logic-op defeats the purpose of the metadata. +; FIXME: Merging the icmps with logic-op defeats the purpose of the metadata. ; We can't tell which condition is expensive if they are combined. define void @or_icmps_harmful(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @or_icmps_harmful( ; CHECK-NEXT: entry: ; CHECK-NEXT: [[EXPECTED_TRUE:%.*]] = icmp sgt i32 [[X:%.*]], -1 -; CHECK-NEXT: br i1 [[EXPECTED_TRUE]], label [[EXIT:%.*]], label [[RARE:%.*]], !prof !19 -; CHECK: rare: ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 -; CHECK-NEXT: br i1 [[EXPENSIVE]], label [[EXIT]], label [[FALSE:%.*]] +; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[EXPECTED_TRUE]], [[EXPENSIVE]] +; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !19 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -669,17 +668,16 @@ exit: ret void } -; Merging the icmps with logic-op defeats the purpose of the metadata. +; FIXME: Merging the icmps with logic-op defeats the purpose of the metadata. ; We can't tell which condition is expensive if they are combined. define void @or_icmps_harmful_inverted(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @or_icmps_harmful_inverted( ; CHECK-NEXT: entry: -; CHECK-NEXT: [[EXPECTED_FALSE:%.*]] = icmp sgt i32 [[X:%.*]], -1 -; CHECK-NEXT: br i1 [[EXPECTED_FALSE]], label [[RARE:%.*]], label [[EXIT:%.*]], !prof !20 -; CHECK: rare: +; CHECK-NEXT: [[EXPECTED_FALSE:%.*]] = icmp sle i32 [[X:%.*]], -1 ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 -; CHECK-NEXT: br i1 [[EXPENSIVE]], label [[EXIT]], label [[FALSE:%.*]] +; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[EXPECTED_FALSE]], [[EXPENSIVE]] +; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !19 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -702,8 +700,7 @@ exit: ret void } -; The probability threshold is determined by a TTI setting. -; In this example, we are just short of strongly expected, so speculate. +; The probability threshold is set by a builtin_expect setting. define void @or_icmps_not_that_harmful(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @or_icmps_not_that_harmful( @@ -711,7 +708,7 @@ define void @or_icmps_not_that_harmful(i32 %x, i32 %y, i8* %p) { ; CHECK-NEXT: [[EXPECTED_TRUE:%.*]] = icmp sgt i32 [[X:%.*]], -1 ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 ; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[EXPECTED_TRUE]], [[EXPENSIVE]] -; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !21 +; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !20 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -734,16 +731,13 @@ exit: ret void } -; The probability threshold is determined by a TTI setting. -; In this example, we are just short of strongly expected, so speculate. - define void @or_icmps_not_that_harmful_inverted(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @or_icmps_not_that_harmful_inverted( ; CHECK-NEXT: entry: ; CHECK-NEXT: [[EXPECTED_TRUE:%.*]] = icmp sgt i32 [[X:%.*]], -1 ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 ; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[EXPECTED_TRUE]], [[EXPENSIVE]] -; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !22 +; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !21 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -766,15 +760,13 @@ exit: ret void } -; The 1st cmp is probably true, so speculating the 2nd is probably a win. - define void @or_icmps_useful(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @or_icmps_useful( ; CHECK-NEXT: entry: ; CHECK-NEXT: [[EXPECTED_TRUE:%.*]] = icmp sle i32 [[X:%.*]], -1 ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 ; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[EXPECTED_TRUE]], [[EXPENSIVE]] -; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !23 +; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !22 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -797,15 +789,13 @@ exit: ret void } -; The 1st cmp is probably false, so speculating the 2nd is probably a win. - define void @or_icmps_useful_inverted(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @or_icmps_useful_inverted( ; CHECK-NEXT: entry: ; CHECK-NEXT: [[EXPECTED_FALSE:%.*]] = icmp sgt i32 [[X:%.*]], -1 ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 ; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[EXPECTED_FALSE]], [[EXPENSIVE]] -; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !23 +; CHECK-NEXT: br i1 [[OR_COND]], label [[EXIT:%.*]], label [[FALSE:%.*]], !prof !22 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -859,17 +849,16 @@ exit: ret void } -; Merging the icmps with logic-op defeats the purpose of the metadata. +; FIXME: Merging the icmps with logic-op defeats the purpose of the metadata. ; We can't tell which condition is expensive if they are combined. define void @and_icmps_harmful(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @and_icmps_harmful( ; CHECK-NEXT: entry: ; CHECK-NEXT: [[EXPECTED_FALSE:%.*]] = icmp sgt i32 [[X:%.*]], -1 -; CHECK-NEXT: br i1 [[EXPECTED_FALSE]], label [[RARE:%.*]], label [[EXIT:%.*]], !prof !20 -; CHECK: rare: ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 -; CHECK-NEXT: br i1 [[EXPENSIVE]], label [[FALSE:%.*]], label [[EXIT]] +; CHECK-NEXT: [[OR_COND:%.*]] = and i1 [[EXPECTED_FALSE]], [[EXPENSIVE]] +; CHECK-NEXT: br i1 [[OR_COND]], label [[FALSE:%.*]], label [[EXIT:%.*]], !prof !23 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -892,17 +881,16 @@ exit: ret void } -; Merging the icmps with logic-op defeats the purpose of the metadata. +; FIXME: Merging the icmps with logic-op defeats the purpose of the metadata. ; We can't tell which condition is expensive if they are combined. define void @and_icmps_harmful_inverted(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @and_icmps_harmful_inverted( ; CHECK-NEXT: entry: -; CHECK-NEXT: [[EXPECTED_TRUE:%.*]] = icmp sgt i32 [[X:%.*]], -1 -; CHECK-NEXT: br i1 [[EXPECTED_TRUE]], label [[EXIT:%.*]], label [[RARE:%.*]], !prof !19 -; CHECK: rare: +; CHECK-NEXT: [[EXPECTED_TRUE:%.*]] = icmp sle i32 [[X:%.*]], -1 ; CHECK-NEXT: [[EXPENSIVE:%.*]] = icmp eq i32 [[Y:%.*]], 0 -; CHECK-NEXT: br i1 [[EXPENSIVE]], label [[FALSE:%.*]], label [[EXIT]] +; CHECK-NEXT: [[OR_COND:%.*]] = and i1 [[EXPECTED_TRUE]], [[EXPENSIVE]] +; CHECK-NEXT: br i1 [[OR_COND]], label [[FALSE:%.*]], label [[EXIT:%.*]], !prof !23 ; CHECK: false: ; CHECK-NEXT: store i8 42, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br label [[EXIT]] @@ -925,9 +913,6 @@ exit: ret void } -; The probability threshold is determined by a TTI setting. -; In this example, we are just short of strongly expected, so speculate. - define void @and_icmps_not_that_harmful(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @and_icmps_not_that_harmful( ; CHECK-NEXT: entry: @@ -957,9 +942,6 @@ exit: ret void } -; The probability threshold is determined by a TTI setting. -; In this example, we are just short of strongly expected, so speculate. - define void @and_icmps_not_that_harmful_inverted(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @and_icmps_not_that_harmful_inverted( ; CHECK-NEXT: entry: @@ -989,8 +971,6 @@ exit: ret void } -; The 1st cmp is probably true, so speculating the 2nd is probably a win. - define void @and_icmps_useful(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @and_icmps_useful( ; CHECK-NEXT: entry: @@ -1020,8 +1000,6 @@ exit: ret void } -; The 1st cmp is probably false, so speculating the 2nd is probably a win. - define void @and_icmps_useful_inverted(i32 %x, i32 %y, i8* %p) { ; CHECK-LABEL: @and_icmps_useful_inverted( ; CHECK-NEXT: entry: