From c98d1ca574fb64659e373c0a5629cb88ef176df4 Mon Sep 17 00:00:00 2001 From: zoecarver Date: Sat, 11 Jan 2020 14:10:07 -0800 Subject: [PATCH 1/5] Refactor in prep for opt --- .../SILCombiner/SILCombinerApplyVisitors.cpp | 103 ++++++++++-------- 1 file changed, 58 insertions(+), 45 deletions(-) diff --git a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp index 90607e637f7d9..1cdaab10f61e5 100644 --- a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp +++ b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp @@ -212,6 +212,55 @@ static void insertEndAccess(BeginAccessInst *&beginAccess, bool isModify, } } +static SILValue createKeypathStoredPropertyProjections(SILValue addr, + SILLocation loc, + BeginAccessInst *&beginAccess, + SILBuilder &builder, + const KeyPathPatternComponent& comp) { + assert(comp.getKind() == KeyPathPatternComponent::Kind::StoredProperty); + VarDecl *storedProperty = comp.getStoredPropertyDecl(); + SILValue elementAddr; + if (addr->getType().getStructOrBoundGenericStruct()) { + addr = builder.createStructElementAddr(loc, addr, storedProperty); + } else if (addr->getType().getClassOrBoundGenericClass()) { + SingleValueInstruction *Ref = builder.createLoad(loc, addr, + LoadOwnershipQualifier::Unqualified); + insertEndAccess(beginAccess, /*isModify*/ false, builder); + + // Handle the case where the storedProperty is in a super class. + while (Ref->getType().getClassOrBoundGenericClass() != + storedProperty->getDeclContext()) { + SILType superCl = Ref->getType().getSuperclass(); + if (!superCl) { + // This should never happen, because the property should be in the + // decl or in a superclass of it. Just handle this to be on the safe + // side. + return SILValue(); + } + Ref = builder.createUpcast(loc, Ref, superCl); + } + + addr = builder.createRefElementAddr(loc, Ref, storedProperty); + + // Class members need access enforcement. + if (builder.getModule().getOptions().EnforceExclusivityDynamic) { + beginAccess = builder.createBeginAccess(loc, addr, SILAccessKind::Read, + SILAccessEnforcement::Dynamic, + /*noNestedConflict*/ false, + /*fromBuiltin*/ false); + addr = beginAccess; + } + } else { + // This should never happen, as a stored-property pattern can only be + // applied to classes and structs. But to be safe - and future prove - + // let's handle this case and bail. + insertEndAccess(beginAccess, /*isModify*/ false, builder); + return SILValue(); + } + + return addr; +} + /// Creates the projection pattern for a keypath instruction. /// /// Currently only the StoredProperty pattern is handled. @@ -233,55 +282,19 @@ static SILValue createKeypathProjections(SILValue keyPath, SILValue root, auto components = kpInst->getPattern()->getComponents(); - // Check if the keypath only contains patterns which we support. - for (const KeyPathPatternComponent &comp : components) { - if (comp.getKind() != KeyPathPatternComponent::Kind::StoredProperty) - return SILValue(); - } - SILValue addr = root; + // Check if the keypath only contains patterns which we support. for (const KeyPathPatternComponent &comp : components) { - assert(comp.getKind() == KeyPathPatternComponent::Kind::StoredProperty); - VarDecl *storedProperty = comp.getStoredPropertyDecl(); - SILValue elementAddr; - if (addr->getType().getStructOrBoundGenericStruct()) { - addr = builder.createStructElementAddr(loc, addr, storedProperty); - } else if (addr->getType().getClassOrBoundGenericClass()) { - SingleValueInstruction *Ref = builder.createLoad(loc, addr, - LoadOwnershipQualifier::Unqualified); - insertEndAccess(beginAccess, /*isModify*/ false, builder); - - // Handle the case where the storedProperty is in a super class. - while (Ref->getType().getClassOrBoundGenericClass() != - storedProperty->getDeclContext()) { - SILType superCl = Ref->getType().getSuperclass(); - if (!superCl) { - // This should never happen, because the property should be in the - // decl or in a superclass of it. Just handle this to be on the safe - // side. - return SILValue(); - } - Ref = builder.createUpcast(loc, Ref, superCl); - } - - addr = builder.createRefElementAddr(loc, Ref, storedProperty); - - // Class members need access enforcement. - if (builder.getModule().getOptions().EnforceExclusivityDynamic) { - beginAccess = builder.createBeginAccess(loc, addr, SILAccessKind::Read, - SILAccessEnforcement::Dynamic, - /*noNestedConflict*/ false, - /*fromBuiltin*/ false); - addr = beginAccess; - } - } else { - // This should never happen, as a stored-property pattern can only be - // applied to classes and structs. But to be safe - and future prove - - // let's handle this case and bail. - insertEndAccess(beginAccess, /*isModify*/ false, builder); - return SILValue(); + switch (comp.getKind()) { + case KeyPathPatternComponent::Kind::StoredProperty: + addr = createKeypathStoredPropertyProjections(addr, loc, beginAccess, + builder, comp); + break; + default: + return SILValue(); } } + return addr; } From dfcb088ccf413cef104d8ab77a1fabd4d815d162 Mon Sep 17 00:00:00 2001 From: zoecarver Date: Sun, 12 Jan 2020 13:43:29 -0800 Subject: [PATCH 2/5] Finish keypath getter opt --- .../SILCombiner/SILCombinerApplyVisitors.cpp | 63 ++++++++++++++++--- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp index 1cdaab10f61e5..53a32eb2a7389 100644 --- a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp +++ b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp @@ -261,6 +261,41 @@ static SILValue createKeypathStoredPropertyProjections(SILValue addr, return addr; } +static SILValue createKeypathGettablePropertyProjections(KeyPathInst *keyPath, + SILValue addr, + SILValue valueAddr, + SILLocation loc, + BeginAccessInst *&beginAccess, + SILBuilder &builder, + const KeyPathPatternComponent& comp) { + assert(comp.getKind() == KeyPathPatternComponent::Kind::GettableProperty); + + if (!addr->getType().getStructOrBoundGenericStruct() && + !addr->getType().getClassOrBoundGenericClass()) { + // This should never happen, as a stored-property pattern can only be + // applied to classes and structs. But to be safe - and future prove - + // let's handle this case and bail. + insertEndAccess(beginAccess, /*isModify*/ false, builder); + return SILValue(); + } + + SingleValueInstruction *ref = builder.createLoad(loc, addr, + LoadOwnershipQualifier::Unqualified); + + insertEndAccess(beginAccess, /*isModify*/ false, builder); + + SmallVector args; + args.reserve(keyPath->getNumOperands()); + for (auto &op : keyPath->getAllOperands()) { + args.push_back(op.get()); + } + args.push_back(ref); + + auto *getter = comp.getComputedPropertyId().getFunction(); + auto *getterFuncRef = builder.createFunctionRef(loc, getter); + return builder.createApply(loc, getterFuncRef, SubstitutionMap{}, args); +} + /// Creates the projection pattern for a keypath instruction. /// /// Currently only the StoredProperty pattern is handled. @@ -269,6 +304,7 @@ static SILValue createKeypathStoredPropertyProjections(SILValue addr, /// Returns false if \p keyPath is not a keypath instruction or if there is any /// other reason why the optimization cannot be done. static SILValue createKeypathProjections(SILValue keyPath, SILValue root, + SILValue valueAddr, SILLocation loc, BeginAccessInst *&beginAccess, SILBuilder &builder) { @@ -290,6 +326,12 @@ static SILValue createKeypathProjections(SILValue keyPath, SILValue root, addr = createKeypathStoredPropertyProjections(addr, loc, beginAccess, builder, comp); break; + case KeyPathPatternComponent::Kind::GettableProperty: + addr = createKeypathGettablePropertyProjections(cast(keyPath), + addr, valueAddr, + loc, beginAccess, + builder, comp); + break; default: return SILValue(); } @@ -332,19 +374,26 @@ bool SILCombiner::tryOptimizeKeypath(ApplyInst *AI) { } BeginAccessInst *beginAccess = nullptr; - SILValue projectedAddr = createKeypathProjections(keyPath, rootAddr, + SILValue projectedAddr = createKeypathProjections(keyPath, rootAddr, valueAddr, AI->getLoc(), beginAccess, Builder); if (!projectedAddr) return false; - if (isModify) { - Builder.createCopyAddr(AI->getLoc(), valueAddr, projectedAddr, - IsTake, IsNotInitialization); + if (projectedAddr->getType().isAddress()) { + if (isModify) { + Builder.createCopyAddr(AI->getLoc(), valueAddr, projectedAddr, + IsTake, IsNotInitialization); + } else { + Builder.createCopyAddr(AI->getLoc(), projectedAddr, valueAddr, + IsNotTake, IsInitialization); + } } else { - Builder.createCopyAddr(AI->getLoc(), projectedAddr, valueAddr, - IsNotTake, IsInitialization); + assert(valueAddr->getType().isAddress()); + Builder.createStore(AI->getLoc(), projectedAddr, valueAddr, + StoreOwnershipQualifier::Unqualified); } + insertEndAccess(beginAccess, isModify, Builder); eraseInstFromFunction(*AI); ++NumOptimizedKeypaths; @@ -392,7 +441,7 @@ bool SILCombiner::tryOptimizeInoutKeypath(BeginApplyInst *AI) { return false; BeginAccessInst *beginAccess = nullptr; - SILValue projectedAddr = createKeypathProjections(keyPath, rootAddr, + SILValue projectedAddr = createKeypathProjections(keyPath, rootAddr, nullptr, AI->getLoc(), beginAccess, Builder); if (!projectedAddr) From a49fcf4e4fd998b6050a2efdd53b206d116f9c46 Mon Sep 17 00:00:00 2001 From: zoecarver Date: Sun, 12 Jan 2020 14:58:32 -0800 Subject: [PATCH 3/5] Add tests --- .../SILCombiner/SILCombinerApplyVisitors.cpp | 7 +--- test/SILOptimizer/optimize_keypath.swift | 41 +++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp index 53a32eb2a7389..3533e8cd5abfd 100644 --- a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp +++ b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp @@ -263,7 +263,6 @@ static SILValue createKeypathStoredPropertyProjections(SILValue addr, static SILValue createKeypathGettablePropertyProjections(KeyPathInst *keyPath, SILValue addr, - SILValue valueAddr, SILLocation loc, BeginAccessInst *&beginAccess, SILBuilder &builder, @@ -304,7 +303,6 @@ static SILValue createKeypathGettablePropertyProjections(KeyPathInst *keyPath, /// Returns false if \p keyPath is not a keypath instruction or if there is any /// other reason why the optimization cannot be done. static SILValue createKeypathProjections(SILValue keyPath, SILValue root, - SILValue valueAddr, SILLocation loc, BeginAccessInst *&beginAccess, SILBuilder &builder) { @@ -328,8 +326,7 @@ static SILValue createKeypathProjections(SILValue keyPath, SILValue root, break; case KeyPathPatternComponent::Kind::GettableProperty: addr = createKeypathGettablePropertyProjections(cast(keyPath), - addr, valueAddr, - loc, beginAccess, + addr, loc, beginAccess, builder, comp); break; default: @@ -374,7 +371,7 @@ bool SILCombiner::tryOptimizeKeypath(ApplyInst *AI) { } BeginAccessInst *beginAccess = nullptr; - SILValue projectedAddr = createKeypathProjections(keyPath, rootAddr, valueAddr, + SILValue projectedAddr = createKeypathProjections(keyPath, rootAddr, AI->getLoc(), beginAccess, Builder); if (!projectedAddr) diff --git a/test/SILOptimizer/optimize_keypath.swift b/test/SILOptimizer/optimize_keypath.swift index 578270e19d937..18915b9ad1009 100644 --- a/test/SILOptimizer/optimize_keypath.swift +++ b/test/SILOptimizer/optimize_keypath.swift @@ -309,4 +309,45 @@ print("SimpleClass obj count: \(SimpleClass.numObjs)") // CHECK-OUTPUT: GenClass obj count: 0 print("GenClass obj count: \(numGenClassObjs)") +// Optimize subscript getter + +struct Sub { + public let x : Int + public subscript(y: Int, z: Int) -> Int { return x + y + z } + public subscript(y: Int) -> Int { return x + y } + public subscript() -> Int { return x } +} + +// CHECK-LABEL: sil {{.*}}test_subscript_1 +// CHECK: bb0: +// CHECK-NEXT: integer_literal $Builtin.Int64, 42 +// CHECK-NEXT: struct +// CHECK-NEXT: return +public func test_subscript_1() -> Int { + let v = Sub(x: 40) + let xKeyPath = \Sub.[1, 1] + return v[keyPath: xKeyPath] +} + +// CHECK-LABEL: sil {{.*}}test_subscript_2 +// CHECK: bb0: +// CHECK-NEXT: integer_literal $Builtin.Int64, 42 +// CHECK-NEXT: struct +// CHECK-NEXT: return +public func test_subscript_2() -> Int { + let v = Sub(x: 40) + let xKeyPath = \Sub.[2] + return v[keyPath: xKeyPath] +} + +// CHECK-LABEL: sil {{.*}}test_subscript_3 +// CHECK: bb0: +// CHECK-NEXT: integer_literal $Builtin.Int64, 42 +// CHECK-NEXT: struct +// CHECK-NEXT: return +public func test_subscript_3() -> Int { + let v = Sub(x: 42) + let xKeyPath = \Sub.[] + return v[keyPath: xKeyPath] +} From e92678055768a9c73f7b6e02385aa1efcdf32901 Mon Sep 17 00:00:00 2001 From: zoecarver Date: Sun, 12 Jan 2020 14:58:46 -0800 Subject: [PATCH 4/5] Format --- .../SILCombiner/SILCombinerApplyVisitors.cpp | 70 +++++++++---------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp index 3533e8cd5abfd..a4c63714414c1 100644 --- a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp +++ b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp @@ -212,19 +212,17 @@ static void insertEndAccess(BeginAccessInst *&beginAccess, bool isModify, } } -static SILValue createKeypathStoredPropertyProjections(SILValue addr, - SILLocation loc, - BeginAccessInst *&beginAccess, - SILBuilder &builder, - const KeyPathPatternComponent& comp) { +static SILValue createKeypathStoredPropertyProjections( + SILValue addr, SILLocation loc, BeginAccessInst *&beginAccess, + SILBuilder &builder, const KeyPathPatternComponent &comp) { assert(comp.getKind() == KeyPathPatternComponent::Kind::StoredProperty); VarDecl *storedProperty = comp.getStoredPropertyDecl(); SILValue elementAddr; if (addr->getType().getStructOrBoundGenericStruct()) { addr = builder.createStructElementAddr(loc, addr, storedProperty); } else if (addr->getType().getClassOrBoundGenericClass()) { - SingleValueInstruction *Ref = builder.createLoad(loc, addr, - LoadOwnershipQualifier::Unqualified); + SingleValueInstruction *Ref = + builder.createLoad(loc, addr, LoadOwnershipQualifier::Unqualified); insertEndAccess(beginAccess, /*isModify*/ false, builder); // Handle the case where the storedProperty is in a super class. @@ -257,18 +255,16 @@ static SILValue createKeypathStoredPropertyProjections(SILValue addr, insertEndAccess(beginAccess, /*isModify*/ false, builder); return SILValue(); } - + return addr; } -static SILValue createKeypathGettablePropertyProjections(KeyPathInst *keyPath, - SILValue addr, - SILLocation loc, - BeginAccessInst *&beginAccess, - SILBuilder &builder, - const KeyPathPatternComponent& comp) { +static SILValue createKeypathGettablePropertyProjections( + KeyPathInst *keyPath, SILValue addr, SILLocation loc, + BeginAccessInst *&beginAccess, SILBuilder &builder, + const KeyPathPatternComponent &comp) { assert(comp.getKind() == KeyPathPatternComponent::Kind::GettableProperty); - + if (!addr->getType().getStructOrBoundGenericStruct() && !addr->getType().getClassOrBoundGenericClass()) { // This should never happen, as a stored-property pattern can only be @@ -277,10 +273,10 @@ static SILValue createKeypathGettablePropertyProjections(KeyPathInst *keyPath, insertEndAccess(beginAccess, /*isModify*/ false, builder); return SILValue(); } - - SingleValueInstruction *ref = builder.createLoad(loc, addr, - LoadOwnershipQualifier::Unqualified); - + + SingleValueInstruction *ref = + builder.createLoad(loc, addr, LoadOwnershipQualifier::Unqualified); + insertEndAccess(beginAccess, /*isModify*/ false, builder); SmallVector args; @@ -289,7 +285,7 @@ static SILValue createKeypathGettablePropertyProjections(KeyPathInst *keyPath, args.push_back(op.get()); } args.push_back(ref); - + auto *getter = comp.getComputedPropertyId().getFunction(); auto *getterFuncRef = builder.createFunctionRef(loc, getter); return builder.createApply(loc, getterFuncRef, SubstitutionMap{}, args); @@ -320,17 +316,16 @@ static SILValue createKeypathProjections(SILValue keyPath, SILValue root, // Check if the keypath only contains patterns which we support. for (const KeyPathPatternComponent &comp : components) { switch (comp.getKind()) { - case KeyPathPatternComponent::Kind::StoredProperty: - addr = createKeypathStoredPropertyProjections(addr, loc, beginAccess, - builder, comp); - break; - case KeyPathPatternComponent::Kind::GettableProperty: - addr = createKeypathGettablePropertyProjections(cast(keyPath), - addr, loc, beginAccess, - builder, comp); - break; - default: - return SILValue(); + case KeyPathPatternComponent::Kind::StoredProperty: + addr = createKeypathStoredPropertyProjections(addr, loc, beginAccess, + builder, comp); + break; + case KeyPathPatternComponent::Kind::GettableProperty: + addr = createKeypathGettablePropertyProjections( + cast(keyPath), addr, loc, beginAccess, builder, comp); + break; + default: + return SILValue(); } } @@ -379,11 +374,11 @@ bool SILCombiner::tryOptimizeKeypath(ApplyInst *AI) { if (projectedAddr->getType().isAddress()) { if (isModify) { - Builder.createCopyAddr(AI->getLoc(), valueAddr, projectedAddr, - IsTake, IsNotInitialization); + Builder.createCopyAddr(AI->getLoc(), valueAddr, projectedAddr, IsTake, + IsNotInitialization); } else { - Builder.createCopyAddr(AI->getLoc(), projectedAddr, valueAddr, - IsNotTake, IsInitialization); + Builder.createCopyAddr(AI->getLoc(), projectedAddr, valueAddr, IsNotTake, + IsInitialization); } } else { assert(valueAddr->getType().isAddress()); @@ -438,9 +433,8 @@ bool SILCombiner::tryOptimizeInoutKeypath(BeginApplyInst *AI) { return false; BeginAccessInst *beginAccess = nullptr; - SILValue projectedAddr = createKeypathProjections(keyPath, rootAddr, nullptr, - AI->getLoc(), beginAccess, - Builder); + SILValue projectedAddr = createKeypathProjections( + keyPath, rootAddr, nullptr, AI->getLoc(), beginAccess, Builder); if (!projectedAddr) return false; From 5cc56c77f952b3159ac21534d8ab719bf16f185c Mon Sep 17 00:00:00 2001 From: zoecarver Date: Sun, 12 Jan 2020 15:00:56 -0800 Subject: [PATCH 5/5] Call correct function in inout opt --- lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp index a4c63714414c1..1f5b4793f5e49 100644 --- a/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp +++ b/lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp @@ -434,7 +434,7 @@ bool SILCombiner::tryOptimizeInoutKeypath(BeginApplyInst *AI) { BeginAccessInst *beginAccess = nullptr; SILValue projectedAddr = createKeypathProjections( - keyPath, rootAddr, nullptr, AI->getLoc(), beginAccess, Builder); + keyPath, rootAddr, AI->getLoc(), beginAccess, Builder); if (!projectedAddr) return false;