From eb698970dd34e73d9761e345e2b655205aa02354 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 26 Jun 2023 17:08:06 -0700 Subject: [PATCH 1/4] [MoveOnlyAddressChecker] NFC: Used helper. Used the TypeTreeLeafTypeRange::setBits helper rather than looping over the range and setting the bits in place. --- lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp index 1861a6478a88a..ddfd908a58f73 100644 --- a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp +++ b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp @@ -668,9 +668,7 @@ struct UseState { void recordLivenessUse(SILInstruction *inst, TypeTreeLeafTypeRange range) { auto &bits = getOrCreateLivenessUse(inst); - for (auto element : range.getRange()) { - bits.set(element); - } + range.setBits(bits); } /// Returns true if this is a terminator instruction that although it doesn't From 50798ff67ed2eca0649ceb64aeffe78b2c24ccb1 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 26 Jun 2023 17:11:32 -0700 Subject: [PATCH 2/4] [MoveOnlyAddressChecker] NFC: Extracted function. In preparation to share the getOrCreateConsumingBlock functionality with another overload of recordConsumingBlock. --- .../Mandatory/MoveOnlyAddressCheckerUtils.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp index ddfd908a58f73..392f21d5d66d9 100644 --- a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp +++ b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp @@ -763,14 +763,19 @@ struct UseState { } } - void recordConsumingBlock(SILBasicBlock *block, TypeTreeLeafTypeRange range) { + SmallBitVector &getOrCreateConsumingBlock(SILBasicBlock *block) { auto iter = consumingBlocks.find(block); if (iter == consumingBlocks.end()) { iter = consumingBlocks.insert({block, SmallBitVector(getNumSubelements())}) .first; } - range.setBits(iter->second); + return iter->second; + } + + void recordConsumingBlock(SILBasicBlock *block, TypeTreeLeafTypeRange range) { + auto &consumingBits = getOrCreateConsumingBlock(block); + range.setBits(consumingBits); } void From 0b30bd736138caea6d131516686007697f80bec4 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 26 Jun 2023 17:30:09 -0700 Subject: [PATCH 3/4] [FieldSensitivePL] NFC: Added initDef(bit vector). The new overload iterates over the contiguous ranges in the bit vector and calls through to the overload that takes a range. --- .../swift/SIL/FieldSensitivePrunedLiveness.h | 9 ++++++++ .../Utils/FieldSensitivePrunedLiveness.cpp | 23 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/include/swift/SIL/FieldSensitivePrunedLiveness.h b/include/swift/SIL/FieldSensitivePrunedLiveness.h index 345fdec8faf5c..dc6a1ee980159 100644 --- a/include/swift/SIL/FieldSensitivePrunedLiveness.h +++ b/include/swift/SIL/FieldSensitivePrunedLiveness.h @@ -321,6 +321,10 @@ struct TypeTreeLeafTypeRange { SmallVectorImpl> &resultingProjections); + static void visitContiguousRanges( + SmallBitVector const &bits, + llvm::function_ref callback); + bool operator==(const TypeTreeLeafTypeRange &other) const { return startEltOffset == other.startEltOffset && endEltOffset == other.endEltOffset; @@ -1217,6 +1221,11 @@ class FieldSensitiveMultiDefPrunedLiveRange defBlocks.setFrozen(); } + void initializeDef(SILInstruction *def, SmallBitVector const &bits) { + TypeTreeLeafTypeRange::visitContiguousRanges( + bits, [&](auto range) { initializeDef(def, range); }); + } + void initializeDef(SILValue def, TypeTreeLeafTypeRange span) { assert(Super::isInitialized()); defs.insert(def, span); diff --git a/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp b/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp index aa22c01a495d6..fde3c2a241cc1 100644 --- a/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp +++ b/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp @@ -474,6 +474,29 @@ void TypeTreeLeafTypeRange::constructProjectionsForNeededElements( } } +void TypeTreeLeafTypeRange::visitContiguousRanges( + SmallBitVector const &bits, + llvm::function_ref callback) { + if (bits.size() == 0) + return; + + llvm::Optional current = llvm::None; + for (unsigned bit = 0, size = bits.size(); bit < size; ++bit) { + auto isSet = bits.test(bit); + if (current) { + if (!isSet) { + callback(TypeTreeLeafTypeRange(*current, bit)); + current = llvm::None; + } + } else if (isSet) { + current = bit; + } + } + if (current) { + callback(TypeTreeLeafTypeRange(*current, bits.size())); + } +} + //===----------------------------------------------------------------------===// // MARK: FieldSensitivePrunedLiveBlocks //===----------------------------------------------------------------------===// From 4a5009a612a012d85c1a9dc4b3f1bc1ff1f160e8 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 26 Jun 2023 17:33:30 -0700 Subject: [PATCH 4/4] [MoveOnlyAddressChecker] Fix repr for reinits. The address checker records instructions that reinit fields in its reinitInsts map. Previously, that map mapped from an instruction to a range of fields of the type. But an instruction can use multiple discontiguous fields of a single value. (Indeed an attempt to add a second range that was reinit'd by an already reinit'ing instruction--even if it were overlapping or adjacent--would have no effect and the map wouldn't be updated.) Here, this is fixed by fixing the representation and updating the storage whenver a new range is seen to be reinit'd by the instruction. As in https://github.com/apple/swift/pull/66728 , a SmallBitVector is the representation chosen. rdar://111356251 --- .../Mandatory/MoveOnlyAddressCheckerUtils.cpp | 30 +++++++++---- .../moveonly_addresschecker_unmaximized.sil | 44 ++++++++++++++++++- 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp index 392f21d5d66d9..493560ee350c7 100644 --- a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp +++ b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp @@ -626,7 +626,7 @@ struct UseState { /// memInstMustReinitialize insts. Contains both insts like copy_addr/store /// [assign] that are reinits that we will convert to inits and true reinits. - llvm::SmallMapVector reinitInsts; + llvm::SmallMapVector reinitInsts; /// The set of drop_deinits of this mark_must_check SmallSetVector dropDeinitInsts; @@ -671,6 +671,16 @@ struct UseState { range.setBits(bits); } + void recordReinitUse(SILInstruction *inst, TypeTreeLeafTypeRange range) { + auto iter = reinitInsts.find(inst); + if (iter == reinitInsts.end()) { + iter = + reinitInsts.insert({inst, SmallBitVector(getNumSubelements())}).first; + } + auto &bits = iter->second; + range.setBits(bits); + } + /// Returns true if this is a terminator instruction that although it doesn't /// use our inout argument directly is used by the pass to ensure that we /// reinit said argument if we consumed it in the body of the function. @@ -778,6 +788,11 @@ struct UseState { range.setBits(consumingBits); } + void recordConsumingBlock(SILBasicBlock *block, SmallBitVector &bits) { + auto &consumingBits = getOrCreateConsumingBlock(block); + consumingBits |= bits; + } + void initializeLiveness(FieldSensitiveMultiDefPrunedLiveRange &prunedLiveness); @@ -842,7 +857,7 @@ struct UseState { if (!isReinitToInitConvertibleInst(inst)) { auto iter = reinitInsts.find(inst); if (iter != reinitInsts.end()) { - if (span.setIntersection(iter->second)) + if (span.intersects(iter->second)) return true; } } @@ -867,7 +882,7 @@ struct UseState { if (isReinitToInitConvertibleInst(inst)) { auto iter = reinitInsts.find(inst); if (iter != reinitInsts.end()) { - if (span.setIntersection(iter->second)) + if (span.intersects(iter->second)) return true; } } @@ -1744,11 +1759,10 @@ bool GatherUsesVisitor::visitUse(Operand *op) { if (::memInstMustReinitialize(op)) { LLVM_DEBUG(llvm::dbgs() << "Found reinit: " << *user); - assert(!useState.reinitInsts.count(user)); auto leafRange = TypeTreeLeafTypeRange::get(op->get(), getRootAddress()); if (!leafRange) return false; - useState.reinitInsts.insert({user, *leafRange}); + useState.recordReinitUse(user, *leafRange); return true; } @@ -2733,9 +2747,7 @@ void MoveOnlyAddressCheckerPImpl::rewriteUses( for (auto reinitPair : addressUseState.reinitInsts) { if (!isReinitToInitConvertibleInst(reinitPair.first)) continue; - SmallBitVector bits(liveness.getNumSubElements()); - reinitPair.second.setBits(bits); - if (!consumes.claimConsume(reinitPair.first, bits)) + if (!consumes.claimConsume(reinitPair.first, reinitPair.second)) convertMemoryReinitToInitForm(reinitPair.first, debugVar); } @@ -2926,7 +2938,7 @@ void ExtendUnconsumedLiveness::run() { } } for (auto pair : addressUseState.reinitInsts) { - if (pair.second.contains(element)) { + if (pair.second.test(element)) { destroys[pair.first] = DestroyKind::Reinit; } } diff --git a/test/SILOptimizer/moveonly_addresschecker_unmaximized.sil b/test/SILOptimizer/moveonly_addresschecker_unmaximized.sil index 530828ac39ce0..68fe941976da4 100644 --- a/test/SILOptimizer/moveonly_addresschecker_unmaximized.sil +++ b/test/SILOptimizer/moveonly_addresschecker_unmaximized.sil @@ -16,7 +16,7 @@ struct M4 { sil @get_M4 : $@convention(thin) () -> @owned M4 sil @end_2 : $@convention(thin) (@owned M, @owned M) -> () sil @see_addr_2 : $@convention(thin) (@in_guaranteed M, @in_guaranteed M) -> () - +sil @replace_2 : $@convention(thin) (@inout M, @inout M) -> () /// Two non-contiguous fields (#M4.s2, #M4.s4) are borrowed by @see_addr_2. /// Two non-contiguous fields (#M4.s1, #M$.s3) are consumed by @end_2. @@ -65,3 +65,45 @@ bb0: return %22 : $() } +// CHECK-LABEL: sil [ossa] @rdar111356251 : $@convention(thin) () -> () { +// CHECK: [[STACK:%[^,]+]] = alloc_stack $M4 +// CHECK: [[GET_M4:%[^,]+]] = function_ref @get_M4 : $@convention(thin) () -> @owned M4 +// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET_M4]]() : $@convention(thin) () -> @owned M4 +// CHECK: store [[INSTANCE]] to [init] [[STACK]] : $*M4 +// CHECK: [[S2_ADDR:%[^,]+]] = struct_element_addr [[STACK]] : $*M4, #M4.s2 +// CHECK: [[S4_ADDR:%[^,]+]] = struct_element_addr [[STACK]] : $*M4, #M4.s4 +// CHECK: [[REPLACE_2:%[^,]+]] = function_ref @replace_2 : $@convention(thin) (@inout M, @inout M) -> () +// CHECK: apply [[REPLACE_2]]([[S2_ADDR]], [[S4_ADDR]]) : $@convention(thin) (@inout M, @inout M) -> () +// CHECK: [[S4_ADDR_2:%[^,]+]] = struct_element_addr [[STACK]] : $*M4, #M4.s4 +// CHECK: destroy_addr [[S4_ADDR_2]] : $*M +// CHECK: [[S2_ADDR_2:%[^,]+]] = struct_element_addr [[STACK]] : $*M4, #M4.s2 +// CHECK: destroy_addr [[S2_ADDR_2]] : $*M +// CHECK: [[S1_ADDR:%[^,]+]] = struct_element_addr [[STACK]] : $*M4, #M4.s1 +// CHECK: [[S1:%[^,]+]] = load [take] [[S1_ADDR]] : $*M +// CHECK: [[S3_ADDR:%[^,]+]] = struct_element_addr [[STACK]] : $*M4, #M4.s3 +// CHECK: [[S3:%[^,]+]] = load [take] [[S3_ADDR]] : $*M +// CHECK: [[END_2:%[^,]+]] = function_ref @end_2 : $@convention(thin) (@owned M, @owned M) -> () +// CHECK: apply [[END_2]]([[S1]], [[S3]]) : $@convention(thin) (@owned M, @owned M) -> () +// CHECK-LABEL: } // end sil function 'rdar111356251' +sil [ossa] @rdar111356251 : $@convention(thin) () -> () { +bb0: + %stack_addr = alloc_stack $M4 + %stack = mark_must_check [consumable_and_assignable] %stack_addr : $*M4 + %get_M4 = function_ref @get_M4 : $@convention(thin) () -> @owned M4 + %instance = apply %get_M4() : $@convention(thin) () -> @owned M4 + store %instance to [init] %stack : $*M4 + %s2_addr = struct_element_addr %stack : $*M4, #M4.s2 + %s4_addr = struct_element_addr %stack : $*M4, #M4.s4 + %replace_2 = function_ref @replace_2 : $@convention(thin) (@inout M, @inout M) -> () + apply %replace_2(%s2_addr, %s4_addr) : $@convention(thin) (@inout M, @inout M) -> () + %12 = struct_element_addr %stack : $*M4, #M4.s1 + %13 = load [copy] %12 : $*M + %14 = struct_element_addr %stack : $*M4, #M4.s3 + %15 = load [copy] %14 : $*M + %16 = function_ref @end_2 : $@convention(thin) (@owned M, @owned M) -> () + %17 = apply %16(%13, %15) : $@convention(thin) (@owned M, @owned M) -> () + destroy_addr %stack : $*M4 + dealloc_stack %stack_addr : $*M4 + %22 = tuple () + return %22 : $() +}