BoundsCheckOpts: merge OutputSpan.append capacity checks#89394
Open
airspeedswift wants to merge 1 commit into
Open
BoundsCheckOpts: merge OutputSpan.append capacity checks#89394airspeedswift wants to merge 1 commit into
airspeedswift wants to merge 1 commit into
Conversation
Member
Author
|
@swift-ci please smoke test |
Member
Author
|
with swiftlang/sourcekit-lsp#2666 @swift-ci Please smoke test Windows platform |
meg-gupta
reviewed
May 27, 2026
| // necessary, because we can just reuse the value. Note that if the value is | ||
| // in a different block it must come before the insertion point because the | ||
| // value dominates insertion point. | ||
| if (isa<SILArgument>(value) || |
Contributor
There was a problem hiding this comment.
This early exit skips the conservative safety check inst->mayReadFromMemory() and incorrectly hoists bounds checks for:
sil @mutate_outputspan : $@convention(thin) (@inout OutputSpan<Int>) -> ()
// CHECK-LABEL: sil [ossa] @test_outputspan_check_capacity_across_mutation :
// CHECK: [[CHECK:%[0-9]+]] = function_ref @checkCapacity
// CHECK: apply [[CHECK]]
// CHECK: function_ref @mutate_outputspan
// CHECK: apply
// CHECK: apply [[CHECK]]
// CHECK-LABEL: } // end sil function 'test_outputspan_check_capacity_across_mutation'
sil [ossa] @test_outputspan_check_capacity_across_mutation : $@convention(thin) (@inout OutputSpan<Int>) -> () {
bb0(%0 : $*OutputSpan<Int>):
%self1 = load_borrow %0 : $*OutputSpan<Int>
%idx1_raw = integer_literal $Builtin.Int64, 0
%idx1 = struct $Int (%idx1_raw : $Builtin.Int64)
%check_fn = function_ref @checkCapacity : $@convention(method) (Int, @guaranteed OutputSpan<Int>) -> ()
%c1 = apply %check_fn(%idx1, %self1) : $@convention(method) (Int, @guaranteed OutputSpan<Int>) -> ()
end_borrow %self1 : $OutputSpan<Int>
%mutate_fn = function_ref @mutate_outputspan : $@convention(thin) (@inout OutputSpan<Int>) -> ()
%m = apply %mutate_fn(%0) : $@convention(thin) (@inout OutputSpan<Int>) -> ()
%self2 = load_borrow %0 : $*OutputSpan<Int>
%idx2_raw = integer_literal $Builtin.Int64, 1
%idx2 = struct $Int (%idx2_raw : $Builtin.Int64)
%c2 = apply %check_fn(%idx2, %self2) : $@convention(method) (Int, @guaranteed OutputSpan<Int>) -> ()
end_borrow %self2 : $OutputSpan<Int>
%t = tuple ()
return %t : $()
}
Contributor
There was a problem hiding this comment.
inst->mayReadFromMemory() is also quite conservative and ideally we need to query alias analysis that any instruction we hoist over cannot mutate self of OutputSpan
Sequential `OutputSpan.append` calls each emitted their own `count <
capacity` check, since the inline `_precondition` lowered to `cond_fail`
with no semantic apply for `hoistFixedStorageBoundsChecksInBlock` to
recognise, and successive loads of the OutputSpan struct produced
distinct SSA selves that fragmented the merge group.
Split the check into a `_checkCanAppend` helper tagged
`@_semantics("fixed_storage.check_capacity")`. Teach the pass to look
through `load` / `load_borrow` for the merge-group key when the call
kind is `CheckCapacity`, and to substitute the first check's self when
moving subsequent ones (sound because `capacity` is invariant; the
helper reads no mutable fields). `CheckIndex` keying is unchanged so
reads against mutable `_count` still behave correctly.
Self substitution alone is unsound across instructions that could
replace the storage at the merge-group address — most commonly an
opaque `@inout` callee. Before merging a `CheckCapacity` call, scan the
range from the previous clustered check to the candidate and bail if
any intervening apply (other than a recognized fixed-storage semantic
call) or whole-storage `store` / `copy_addr` lands on the same address.
Field stores reach `struct_element_addr`-derived destinations and do
not bail.
Four sequential appends on the same `inout OutputSpan` now fold to one
merged `cond_fail` covering all four indices, mirroring `span_4_sum`.
891f47b to
cad9d80
Compare
Member
Author
|
@swift-ci please test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sequential
OutputSpan.appendcalls each emitted their owncount < capacitycheck, since the inline_preconditionlowered tocond_failwith no semantic apply forhoistFixedStorageBoundsChecksInBlockto recognise, and successive loads of the OutputSpan struct produced distinct SSA selves that fragmented the merge group.Split the check into a
_checkCanAppendhelper tagged@_semantics("fixed_storage.check_capacity"). Teach the pass to look throughload/load_borrowfor the merge-group key when the call kind isCheckCapacity, and to substitute the first check's self when moving subsequent ones (sound becausecapacityis invariant; the helper reads no mutable fields).CheckIndexkeying is unchanged so reads against mutable_countstill behave correctly.Also tighten
cloneFixedStorageIndexto bail when the index expression depends on a memory read defined after the insertion point. Without this, an intervening@inoutcallee that mutates_countbetween two capacity checks could be skipped over: the clonedloadof_countwould read the pre-mutation value, the moved check would compare the stale count against capacity, and the trap that should fire would be silently elided.Four sequential appends on the same
inout OutputSpannow fold to one mergedcond_failcovering all four indices, mirroringspan_4_sum.Resolves rdar://168782676 (OutputSpan does not hoist capacity check for multiple append calls)