llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-llvm-transforms Author: Aiden Grossman (boomanaiden154) <details> <summary>Changes</summary> This patch fixes the profcheck failure from that patch by explicitly updating the branch weights given we can infer what the values should be. --- Full diff: https://github.com/llvm/llvm-project/pull/220811.diff 2 Files Affected: - (modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+29-2) - (modified) llvm/test/Transforms/SimplifyCFG/switch-select-remap.ll (+19-50) ``````````diff diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index 2a3a88713920b..6f4200a381f6c 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -5098,8 +5098,6 @@ bool SimplifyCFGOpt::simplifySwitchOnSelectRemap(SwitchInst *SI, BasicBlock *BB = SI->getParent(); if (OldDest != DestFork) { - // Case list is changing so we should drop stale profile weights. - SI->setMetadata(LLVMContext::MD_prof, nullptr); if (!IsDefault) OldDest->removePredecessor(BB); if (IsDefault) @@ -5117,6 +5115,35 @@ bool SimplifyCFGOpt::simplifySwitchOnSelectRemap(SwitchInst *SI, if (DTU && !OldDestStillTargeted) DTU->applyUpdates({{DominatorTree::Delete, BB, OldDest}}); } + + // Update the profile information on the switch ifwe had a profile + // for both it and the select instruction. We only need to do this + // in the case where we add a case to the switch. + if (IsDefault) { + SmallVector<uint32_t> SwitchWeights; + bool SwitchHasBranchWeights = extractBranchWeights(*SI, SwitchWeights); + SmallVector<uint32_t> SelectWeights; + bool SelectHasBranchWeights = + extractBranchWeights(*Select, SelectWeights); + if (SwitchHasBranchWeights && SelectHasBranchWeights && + !ProfcheckDisableMetadataFixes) { + // Update the branch weights of the switch by multiplying all of them by + // the weight of the false branch of the select. Then add the new switch + // case at the end by multiplying the weight of the true branch of the + // select by the total weight of the switch. + uint64_t SwitchTotalWeight = sum_of(SwitchWeights, uint64_t{0}); + SmallVector<uint64_t> NewSwitchWeights; + NewSwitchWeights.reserve(SwitchWeights.size() + 1); + for (uint32_t SwitchWeight : SwitchWeights) + NewSwitchWeights.push_back(SwitchWeight * SelectWeights[1]); + NewSwitchWeights.push_back(SwitchTotalWeight * SelectWeights[0]); + setFittedBranchWeights(*SI, NewSwitchWeights, false); + } else if (SwitchHasBranchWeights) { + // If we only have branch weights on the switch, we cannot reconstruct + // branch weights correctly, so mark them as unknown. + setExplicitlyUnknownBranchWeightsIfProfiled(*SI, DEBUG_TYPE); + } + } } // X replaces the condition so compare/select are now dead. diff --git a/llvm/test/Transforms/SimplifyCFG/switch-select-remap.ll b/llvm/test/Transforms/SimplifyCFG/switch-select-remap.ll index 5b20d75ae0d16..07baa71f719b5 100644 --- a/llvm/test/Transforms/SimplifyCFG/switch-select-remap.ll +++ b/llvm/test/Transforms/SimplifyCFG/switch-select-remap.ll @@ -4,14 +4,14 @@ ; The compared value 4 has no explicit case, and the remapped value 6 maps to ; a real (non-default) case, so switching on %x needs a new explicit case for ; 4 pointing to bb2. -define void @test_remap_add_case(i8 %x) { +define void @test_remap_add_case(i8 %x) !prof !0 { ; CHECK-LABEL: define void @test_remap_add_case( -; CHECK-SAME: i8 [[X:%.*]]) { +; CHECK-SAME: i8 [[X:%.*]]) !prof [[PROF0:![0-9]+]] { ; CHECK-NEXT: switch i8 [[X]], label %[[BB1:.*]] [ ; CHECK-NEXT: i8 6, label %[[BB2:.*]] ; CHECK-NEXT: i8 10, label %[[BB3:.*]] ; CHECK-NEXT: i8 4, label %[[BB2]] -; CHECK-NEXT: ] +; CHECK-NEXT: ], !prof [[PROF1:![0-9]+]] ; CHECK: [[BB1]]: ; CHECK-NEXT: call void @func1() ; CHECK-NEXT: unreachable @@ -23,11 +23,11 @@ define void @test_remap_add_case(i8 %x) { ; CHECK-NEXT: unreachable ; %cmp = icmp eq i8 %x, 4 - %key = select i1 %cmp, i8 6, i8 %x + %key = select i1 %cmp, i8 6, i8 %x, !prof !1 switch i8 %key, label %bb1 [ i8 6, label %bb2 i8 10, label %bb3 - ] + ], !prof !2 bb1: call void @func1() @@ -47,14 +47,14 @@ bb3: ; body are removed in the same run: this pass edits the CFG through a ; DomTreeUpdater, so a follow-up SimplifyCFG iteration cleans it up ; immediately instead of needing a separate pass. -define void @test_remap_retarget_case(i8 %x) { +define void @test_remap_retarget_case(i8 %x) !prof !0 { ; CHECK-LABEL: define void @test_remap_retarget_case( -; CHECK-SAME: i8 [[X:%.*]]) { +; CHECK-SAME: i8 [[X:%.*]]) !prof [[PROF0]] { ; CHECK-NEXT: switch i8 [[X]], label %[[BB1:.*]] [ ; CHECK-NEXT: i8 4, label %[[BB2:.*]] ; CHECK-NEXT: i8 6, label %[[BB2]] ; CHECK-NEXT: i8 10, label %[[BB3:.*]] -; CHECK-NEXT: ] +; CHECK-NEXT: ], !prof [[PROF2:![0-9]+]] ; CHECK: [[BB1]]: ; CHECK-NEXT: call void @func1() ; CHECK-NEXT: unreachable @@ -66,12 +66,12 @@ define void @test_remap_retarget_case(i8 %x) { ; CHECK-NEXT: unreachable ; %cmp = icmp eq i8 %x, 4 - %key = select i1 %cmp, i8 6, i8 %x + %key = select i1 %cmp, i8 6, i8 %x, !prof !1 switch i8 %key, label %bb1 [ i8 4, label %bb4 i8 6, label %bb2 i8 10, label %bb3 - ] + ], !prof !3 bb1: call void @func1() @@ -295,45 +295,6 @@ default: unreachable } -; The fold changes the case list (a case may be added, or retargeted to a -; different successor), so any existing branch-weight metadata would -; mislabel the new layout - it must be dropped rather than kept stale. -define void @test_remap_drops_branch_weights(i8 %x) { -; CHECK-LABEL: define void @test_remap_drops_branch_weights( -; CHECK-SAME: i8 [[X:%.*]]) { -; CHECK-NEXT: switch i8 [[X]], label %[[BB1:.*]] [ -; CHECK-NEXT: i8 6, label %[[BB2:.*]] -; CHECK-NEXT: i8 10, label %[[BB3:.*]] -; CHECK-NEXT: i8 4, label %[[BB2]] -; CHECK-NEXT: ] -; CHECK: [[BB1]]: -; CHECK-NEXT: call void @func1() -; CHECK-NEXT: unreachable -; CHECK: [[BB2]]: -; CHECK-NEXT: call void @func2() -; CHECK-NEXT: unreachable -; CHECK: [[BB3]]: -; CHECK-NEXT: call void @func3() -; CHECK-NEXT: unreachable -; - %cmp = icmp eq i8 %x, 4 - %key = select i1 %cmp, i8 6, i8 %x - switch i8 %key, label %bb1 [ - i8 6, label %bb2 - i8 10, label %bb3 - ], !prof !0 - -bb1: - call void @func1() - unreachable -bb2: - call void @func2() - unreachable -bb3: - call void @func3() - unreachable -} - ; Negative test: %key (the select) is used by more than just the switch, so ; folding it away wouldn't actually remove the compare/select sequence - ; leave it alone. @@ -464,4 +425,12 @@ declare void @func3() declare void @func4() declare void @use(i32) -!0 = !{!"branch_weights", i32 1, i32 2, i32 3} +!0 = !{!"function_entry_count", i32 10} +!1 = !{!"branch_weights", i32 2, i32 3} +!2 = !{!"branch_weights", i32 5, i32 7, i32 11} +!3 = !{!"branch_weights", i32 5, i32 7, i32 11, i32 13} +;. +; CHECK: [[PROF0]] = !{!"function_entry_count", i32 10} +; CHECK: [[PROF1]] = !{!"branch_weights", i32 15, i32 21, i32 33, i32 46} +; CHECK: [[PROF2]] = !{!"branch_weights", i32 5, i32 7, i32 11, i32 13} +;. `````````` </details> https://github.com/llvm/llvm-project/pull/220811 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
