From ac3024292bdfbda6173e2526435206ebdaae3672 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Sun, 17 Jul 2016 21:07:51 -0700 Subject: [PATCH] [simplify-cfg] When moving cond_fail to preds, do not bail early. This commit fixes a bug where we were not checking that all predecessors had the cond_fail block as its only successor. This occured since we were bailing early when we saw a constant. So if we saw a predecessor with a constant before a predecessor that had multiple successors, we would optimize even though we would be introducing an extra cond_fail along a path. I added a new utility pass to test this code since so much is going on in SimplifyCFG that it is difficult to construct a test case running the full pass. Really this code should be in a different pass (properly SIL Code Motion TBH). But for now, this commit just fixes the bug. rdar://26904047 --- .../swift/SILOptimizer/PassManager/Passes.def | 3 ++ lib/SILOptimizer/Transforms/SimplifyCFG.cpp | 39 +++++++++++++--- .../move_cond_fail_simplify_cfg.sil | 45 +++++++++++++++++++ 3 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 test/SILOptimizer/move_cond_fail_simplify_cfg.sil diff --git a/include/swift/SILOptimizer/PassManager/Passes.def b/include/swift/SILOptimizer/PassManager/Passes.def index e5b15529b2100..c95dc8c35e469 100644 --- a/include/swift/SILOptimizer/PassManager/Passes.def +++ b/include/swift/SILOptimizer/PassManager/Passes.def @@ -169,6 +169,9 @@ PASS(LSLocationPrinter, "lslocation-dump", "Dump LSLocation results from analyzing all accessed locations") PASS(MergeCondFails, "merge-cond_fails", "Remove redundant overflow checks") +PASS(MoveCondFailToPreds, "move-cond-fail-to-preds", + "Test pass that hoists conditional fails to predecessors blocks when " + "profitable") PASS(NoReturnFolding, "noreturn-folding", "Add 'unreachable' after noreturn calls") PASS(RCIdentityDumper, "rc-id-dumper", diff --git a/lib/SILOptimizer/Transforms/SimplifyCFG.cpp b/lib/SILOptimizer/Transforms/SimplifyCFG.cpp index c1db0a913e5c4..93e992421005e 100644 --- a/lib/SILOptimizer/Transforms/SimplifyCFG.cpp +++ b/lib/SILOptimizer/Transforms/SimplifyCFG.cpp @@ -2121,18 +2121,22 @@ static bool tryMoveCondFailToPreds(SILBasicBlock *BB) { // execute the cond_fail speculatively. if (!Pred->getSingleSuccessor()) return false; - + + // If we already found a constant pred, we do not need to check the incoming + // value to see if it is constant. We are already going to perform the + // optimization. + if (somePredsAreConst) + continue; + SILValue incoming = condArg->getIncomingValue(Pred); - if (isa(incoming)) { - somePredsAreConst = true; - break; - } + somePredsAreConst |= isa(incoming); } + if (!somePredsAreConst) return false; - + DEBUG(llvm::dbgs() << "move to predecessors: " << *CFI); - + // Move the cond_fail to the predecessor blocks. for (auto *Pred : BB->getPreds()) { SILValue incoming = condArg->getIncomingValue(Pred); @@ -3504,6 +3508,22 @@ class SROABBArgs : public SILFunctionTransform { StringRef getName() override { return "SROA BB Arguments"; } }; +// Used to test tryMoveCondFailToPreds with sil-opt +class MoveCondFailToPreds : public SILFunctionTransform { +public: + MoveCondFailToPreds() {} + void run() override { + for (auto &BB : *getFunction()) { + if (tryMoveCondFailToPreds(&BB)) { + invalidateAnalysis( + SILAnalysis::InvalidationKind::BranchesAndInstructions); + } + } + } + + StringRef getName() override { return "Move Cond Fail To Preds"; } +}; + } // End anonymous namespace. /// Splits all critical edges in a function. @@ -3523,3 +3543,8 @@ SILTransform *swift::createSROABBArgs() { return new SROABBArgs(); } SILTransform *swift::createSimplifyBBArgs() { return new SimplifyBBArgs(); } + +// Moves cond_fail instructions to predecessors. +SILTransform *swift::createMoveCondFailToPreds() { + return new MoveCondFailToPreds(); +} diff --git a/test/SILOptimizer/move_cond_fail_simplify_cfg.sil b/test/SILOptimizer/move_cond_fail_simplify_cfg.sil new file mode 100644 index 0000000000000..ede272b6188b6 --- /dev/null +++ b/test/SILOptimizer/move_cond_fail_simplify_cfg.sil @@ -0,0 +1,45 @@ +// RUN: %target-sil-opt -enable-sil-verify-all %s -move-cond-fail-to-preds | FileCheck %s + +import Builtin +import Swift + +sil_stage canonical + +// Make sure that we actually check all predecessors when we try to hoist +// cond_fails. +// +// CHECK-LABEL: sil @check_all_preds_when_hoisting_cond_fails : $@convention(thin) (Builtin.Int1) -> () { +sil @check_all_preds_when_hoisting_cond_fails : $@convention(thin) (Builtin.Int1) -> () { +bb0(%0 : $Builtin.Int1): + %1 = integer_literal $Builtin.Int1, 0 + %2 = integer_literal $Builtin.Int64, 1 + %3 = integer_literal $Builtin.Int64, 3 + cond_br %0, bb3, bb2 + +// Make sure that the predecessor order here is not changed. This is necessary +// for the test to actually test that we are checking /all/ predecessors. +// +// CHECK: bb1({{.*}}): // Preds: bb2 bb1 +bb1(%7 : $Builtin.Int64, %8 : $Builtin.Int64, %9 : $Builtin.Int1): + %10 = builtin "sadd_with_overflow_Int64"(%8 : $Builtin.Int64, %2 : $Builtin.Int64, %1 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1) + %11 = tuple_extract %10 : $(Builtin.Int64, Builtin.Int1), 0 + cond_fail %9 : $Builtin.Int1 + %13 = builtin "sadd_with_overflow_Int64"(%7 : $Builtin.Int64, %2 : $Builtin.Int64, %1 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1) + %14 = tuple_extract %13 : $(Builtin.Int64, Builtin.Int1), 0 + %15 = tuple_extract %13 : $(Builtin.Int64, Builtin.Int1), 1 + cond_fail %15 : $Builtin.Int1 + %17 = builtin "cmp_eq_Int64"(%14 : $Builtin.Int64, %3 : $Builtin.Int64) : $Builtin.Int1 + %18 = builtin "cmp_eq_Int64"(%11 : $Builtin.Int64, %3 : $Builtin.Int64) : $Builtin.Int1 + cond_br %18, bb3, bb1(%14 : $Builtin.Int64, %11 : $Builtin.Int64, %17 : $Builtin.Int1) + +// Make sure there can not be any cond_fail here. +// CHECK: bb2: +// CHECK-NEXT: br bb1 +bb2: + br bb1(%2 : $Builtin.Int64, %2 : $Builtin.Int64, %1 : $Builtin.Int1) + +// CHECK: bb3: +bb3: + %5 = tuple() + return %5 : $() +}