Skip to content

Commit

Permalink
Revert "[SimplifyCFG] use profile metadata to refine merging branch c…
Browse files Browse the repository at this point in the history
…onditions"

This reverts commit 27ae17a.
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.
  • Loading branch information
rotateright committed Mar 22, 2021
1 parent bca0cf7 commit 95f7f7c
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 89 deletions.
59 changes: 18 additions & 41 deletions llvm/lib/Transforms/Utils/SimplifyCFG.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -2841,60 +2840,38 @@ 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<std::pair<Instruction::BinaryOps, bool>>
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();

// Determine if the two branches share a common destination.
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);

Expand Down Expand Up @@ -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;

Expand All @@ -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;
}
Expand Down
14 changes: 7 additions & 7 deletions llvm/test/Transforms/PGOProfile/chr.ll
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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]]
Expand Down Expand Up @@ -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}
60 changes: 19 additions & 41 deletions llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand All @@ -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]]
Expand All @@ -702,16 +700,15 @@ 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(
; 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 !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]]
Expand All @@ -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]]
Expand All @@ -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]]
Expand All @@ -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]]
Expand Down Expand Up @@ -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]]
Expand All @@ -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]]
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit 95f7f7c

Please sign in to comment.