From cf99e5c522ff2dfaabca8b3af2cf4e00ff8fdff7 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Wed, 14 Jun 2017 16:51:12 -0700 Subject: [PATCH] [capture-promotion] When checking if a (struct_element_addr (project_box box)) is written to, check that all of the operands are loads, instead of returning early when we find one. I found this bug by inspection. This is an important bug to fix since this pass runs at -Onone and the bug results in the compiler hitting an unreachable. The way the unreachable is triggered is that when we detect that we are going to promote a box, if we see a (struct_element_addr (project_box box)), we don't map the struct_element_addr to a cloned value. If we have a load, this is not an issue, since we are mapping the load to the struct_extract. But if we have /any/ other non-load users of the struct_element_addr, the cloner will attempt to look up the struct_element_addr and will be unable to find it, hitting an unreachable. rdar://32776202 --- lib/SILOptimizer/IPO/CapturePromotion.cpp | 29 +++++++---- test/SILOptimizer/capture_promotion.swift | 2 +- .../capture_promotion_ownership.sil | 50 +++++++++++++++++++ .../capture_promotion_ownership.swift | 2 +- 4 files changed, 71 insertions(+), 12 deletions(-) diff --git a/lib/SILOptimizer/IPO/CapturePromotion.cpp b/lib/SILOptimizer/IPO/CapturePromotion.cpp index f8b80543a29..4f78846078f 100644 --- a/lib/SILOptimizer/IPO/CapturePromotion.cpp +++ b/lib/SILOptimizer/IPO/CapturePromotion.cpp @@ -694,6 +694,13 @@ static SILArgument *getBoxFromIndex(SILFunction *F, unsigned Index) { return Entry.getArgument(Index); } +static bool isNonMutatingLoad(SILInstruction *I) { + auto *LI = dyn_cast(I); + if (!LI) + return false; + return LI->getOwnershipQualifier() != LoadOwnershipQualifier::Take; +} + /// \brief Given a partial_apply instruction and the argument index into its /// callee's argument list of a box argument (which is followed by an argument /// for the address of the box's contents), return true if the closure is known @@ -726,16 +733,15 @@ isNonMutatingCapture(SILArgument *BoxArg) { // function that mirrors isNonEscapingUse. auto checkAddrUse = [](SILInstruction *AddrInst) { if (auto *SEAI = dyn_cast(AddrInst)) { - for (auto *UseOper : SEAI->getUses()) { - if (isa(UseOper->getUser())) - return true; - } - } else if (isa(AddrInst) || isa(AddrInst) - || isa(AddrInst) - || isa(AddrInst)) { - return true; + return all_of(SEAI->getUses(), + [](Operand *Op) -> bool { + return isNonMutatingLoad(Op->getUser()); + }); } - return false; + + return isNonMutatingLoad(AddrInst) || isa(AddrInst) + || isa(AddrInst) + || isa(AddrInst); }; for (auto *Projection : Projections) { for (auto *UseOper : Projection->getUses()) { @@ -744,7 +750,10 @@ isNonMutatingCapture(SILArgument *BoxArg) { if (!checkAddrUse(AccessUseOper->getUser())) return false; } - } else if (!checkAddrUse(UseOper->getUser())) + continue; + } + + if (!checkAddrUse(UseOper->getUser())) return false; } } diff --git a/test/SILOptimizer/capture_promotion.swift b/test/SILOptimizer/capture_promotion.swift index 331814fae55..09494167900 100644 --- a/test/SILOptimizer/capture_promotion.swift +++ b/test/SILOptimizer/capture_promotion.swift @@ -1,4 +1,4 @@ -// RUN: %target-swift-frontend %s -emit-sil -o - -verify | %FileCheck %s +// RUN: %target-swift-frontend %s -emit-sil -o - | %FileCheck %s class Foo { func foo() -> Int { diff --git a/test/SILOptimizer/capture_promotion_ownership.sil b/test/SILOptimizer/capture_promotion_ownership.sil index 7be1070ea7a..161306f84f3 100644 --- a/test/SILOptimizer/capture_promotion_ownership.sil +++ b/test/SILOptimizer/capture_promotion_ownership.sil @@ -332,3 +332,53 @@ bb0(%0: @trivial $*Int, %1 : @owned $<τ_0_0> { var τ_0_0 } , %2 : @owned %16 = tuple() return %16 : $() } + +// This test makes sure that we properly handle non load uses of +// struct_element_addr that always have load users with the load occuring before +// and after the element in the use list. + +sil @mutate_int : $@convention(thin) (@inout Int) -> () + +// CHECK-LABEL: sil @test_closure_multiple_uses_of_struct_element_addr : $@convention(thin) () -> @owned @callee_owned () -> () { +// CHECK: [[FUNC:%.*]] = function_ref @closure_multiple_uses_of_struct_element_addr : $@convention(thin) +// CHECK: partial_apply [[FUNC]]( +// CHECK: } // end sil function 'test_closure_multiple_uses_of_struct_element_addr' +sil @test_closure_multiple_uses_of_struct_element_addr : $@convention(thin) () -> @owned @callee_owned () -> () { +bb0: + %0 = function_ref @baz_init : $@convention(thin) (@thin Baz.Type) -> @owned Baz + %1 = metatype $@thin Baz.Type + + %2 = alloc_box $<τ_0_0> { var τ_0_0 } + %3 = project_box %2 : $<τ_0_0> { var τ_0_0 } , 0 + %4 = apply %0(%1) : $@convention(thin) (@thin Baz.Type) -> @owned Baz + store %4 to [init] %3 : $*Baz + + %5 = alloc_box $<τ_0_0> { var τ_0_0 } + %6 = project_box %5 : $<τ_0_0> { var τ_0_0 } , 0 + %7 = apply %0(%1) : $@convention(thin) (@thin Baz.Type) -> @owned Baz + store %7 to [init] %6 : $*Baz + + %8 = function_ref @closure_multiple_uses_of_struct_element_addr : $@convention(thin) (@owned <τ_0_0> { var τ_0_0 } , @owned <τ_0_0> { var τ_0_0 } ) -> () + %9 = partial_apply %8(%2, %5) : $@convention(thin) (@owned <τ_0_0> { var τ_0_0 } , @owned <τ_0_0> { var τ_0_0 } ) -> () + + return %9 : $@callee_owned () -> () +} + +sil @closure_multiple_uses_of_struct_element_addr : $@convention(thin) (@owned <τ_0_0> { var τ_0_0 } , @owned <τ_0_0> { var τ_0_0 } ) -> () { +bb0(%0 : @owned $<τ_0_0> { var τ_0_0 } , %1 : @owned $<τ_0_0> { var τ_0_0 } ): + %2 = project_box %0 : $<τ_0_0> { var τ_0_0 } , 0 + %3 = struct_element_addr %2 : $*Baz, #Baz.x + %4 = load [trivial] %3 : $*Int + %5 = function_ref @mutate_int : $@convention(thin) (@inout Int) -> () + apply %5(%3) : $@convention(thin) (@inout Int) -> () + + %6 = project_box %1 : $<τ_0_0> { var τ_0_0 } , 0 + %7 = struct_element_addr %6 : $*Baz, #Baz.x + apply %5(%7) : $@convention(thin) (@inout Int) -> () + %8 = load [trivial] %7 : $*Int + + destroy_value %1 : $<τ_0_0> { var τ_0_0 } + destroy_value %0 : $<τ_0_0> { var τ_0_0 } + %9999 = tuple() + return %9999 : $() +} \ No newline at end of file diff --git a/test/SILOptimizer/capture_promotion_ownership.swift b/test/SILOptimizer/capture_promotion_ownership.swift index 0df992394fa..d9be04b9312 100644 --- a/test/SILOptimizer/capture_promotion_ownership.swift +++ b/test/SILOptimizer/capture_promotion_ownership.swift @@ -1,4 +1,4 @@ -// RUN: %target-swift-frontend %s -enable-sil-ownership -disable-sil-linking -emit-sil -o - -verify | %FileCheck %s +// RUN: %target-swift-frontend %s -enable-sil-ownership -disable-sil-linking -emit-sil -o - | %FileCheck %s // NOTE: We add -disable-sil-linking to the compile line to ensure that we have // access to declarations for standard library types, but not definitions. This