Skip to content

Conversation

meg-gupta
Copy link
Contributor

@meg-gupta meg-gupta commented Jul 9, 2021

We weren't looking at the parameter conventions that resulted in
different side-effects than the defined @effects.

@meg-gupta meg-gupta requested review from gottesmm and atrick July 9, 2021 00:29
@meg-gupta
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strictly better. I don't actually understand the original code though because it isn't well specified. Why isn't an @inout considered a write of the argument?

We weren't looking at the parameter conventions that resulted in
different side-effects than the defined @effects.
@meg-gupta
Copy link
Contributor Author

@swift-ci benchmark

@meg-gupta
Copy link
Contributor Author

meg-gupta commented Jul 26, 2021

@atrick I pushed a version which handles param effects for different conventions. I am seeing some very bad benchmark scores locally, with a lot more retains/releases in the stdlib SIL. Going to do some more investigation before I merge this.

@meg-gupta meg-gupta changed the title Fix SideEffectAnalysis for a function with @effects(readonly) and indirect result Fix SideEffectAnalysis for a function with defined @effects Jul 26, 2021
@swift-ci
Copy link
Contributor

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_insert 2870 5614 +95.6% 0.51x
Set.isDisjoint.Seq.Box0 412 632 +53.4% 0.65x
Set.isDisjoint.Seq.Box25 306 468 +52.9% 0.65x
Set.isSuperset.Seq.Box25 99 151 +52.5% 0.66x
Set.subtracting.Seq.Box0 733 1070 +46.0% 0.69x
Set.isSuperset.Seq.Box0 171 247 +44.4% 0.69x
SetSubtractingBox0 151 214 +41.7% 0.71x
Set.isDisjoint.Box25 364 510 +40.1% 0.71x (?)
Set.isStrictSuperset.Seq.Box25 1074 1478 +37.6% 0.73x
Set.isStrictSubset.Seq.Box0 1074 1477 +37.5% 0.73x
Set.isSubset.Seq.Box0 1082 1477 +36.5% 0.73x
SetUnionBox25 323 440 +36.2% 0.73x
Set.isStrictSuperset.Seq.Box0 10826 14718 +36.0% 0.74x
Set.intersection.Seq.Box0 363 493 +35.8% 0.74x
Set.intersection.Seq.Box25 473 633 +33.8% 0.75x
Set.isStrictSubset.Seq.Box25 1224 1631 +33.3% 0.75x
Set.isSubset.Seq.Box25 1229 1628 +32.5% 0.75x
Set.subtracting.Seq.Box25 1219 1594 +30.8% 0.76x
SetSubtractingBox25 259 329 +27.0% 0.79x
StringFromLongWholeSubstring 4 5 +25.0% 0.80x
DictionaryKeysContainsNative 21 26 +23.8% 0.81x (?)
DataToStringSmall 2350 2900 +23.4% 0.81x (?)
StringEnumRawValueInitialization 600 740 +23.3% 0.81x (?)
SetUnion_OfObjects 5150 6350 +23.3% 0.81x
SetUnionBox0 517 635 +22.8% 0.81x
SetSymmetricDifferenceBox25 440 536 +21.8% 0.82x
SetExclusiveOr_OfObjects 6000 7160 +19.3% 0.84x
SetSymmetricDifferenceBox0 601 716 +19.1% 0.84x
UTF8Decode_InitFromCustom_contiguous_ascii 263 298 +13.3% 0.88x (?)
UTF8Decode_InitFromBytes 196 222 +13.3% 0.88x (?)
SetIntersectionBox25 241 269 +11.6% 0.90x (?)
DictionaryBridgeToObjC_BulkAccess 147 159 +8.2% 0.92x (?)
DictionaryKeysContainsCocoa 25 27 +8.0% 0.93x (?)
DataToStringLargeUnicode 6300 6800 +7.9% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
InsertCharacterEndIndexNonASCII 63 58 -7.9% 1.09x (?)
Set.isDisjoint.Seq.Box.Empty 150 140 -6.7% 1.07x (?)
Array2D 7216 6736 -6.7% 1.07x (?)

Code size: -O

Regression OLD NEW DELTA RATIO
WordCount.o 37505 37905 +1.1% 0.99x
DictTest4.o 15298 15458 +1.0% 0.99x

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
Set.isSubset.Seq.Box0 1310 1833 +39.9% 0.71x
Set.isStrictSuperset.Seq.Box0 13017 18204 +39.8% 0.72x
Set.isStrictSubset.Seq.Box0 1311 1832 +39.7% 0.72x
Set.isStrictSuperset.Seq.Box25 1319 1832 +38.9% 0.72x
Set.isSubset.Seq.Box25 1479 1991 +34.6% 0.74x
Set.isStrictSubset.Seq.Box25 1480 1992 +34.6% 0.74x
Set.intersection.Seq.Box0 404 508 +25.7% 0.80x
StringFromLongWholeSubstring 4 5 +25.0% 0.80x
Set.intersection.Seq.Box25 536 653 +21.8% 0.82x
SetSubtractingBox0 177 213 +20.3% 0.83x (?)
StringEnumRawValueInitialization 600 720 +20.0% 0.83x
Set.subtracting.Seq.Box0 890 1063 +19.4% 0.84x
SetSubtractingBox25 289 330 +14.2% 0.88x
Set.subtracting.Seq.Box25 1409 1595 +13.2% 0.88x (?)
SetUnionBox25 409 449 +9.8% 0.91x (?)
StringBuilderWithLongSubstring 1530 1660 +8.5% 0.92x (?)
UTF8Decode_InitDecoding_ascii_as_ascii 260 282 +8.5% 0.92x (?)
SetUnion_OfObjects 5930 6410 +8.1% 0.93x (?)
SetUnionBox0 593 641 +8.1% 0.93x (?)
SetSymmetricDifferenceBox25 514 553 +7.6% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryBridgeToObjC_Access 975 880 -9.7% 1.11x (?)
Array2D 7520 6944 -7.7% 1.08x (?)
RandomShuffleLCG2 448 416 -7.1% 1.08x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
ObjectiveCBridgeStubFromNSDate 6230 7290 +17.0% 0.85x (?)
RandomTree.insert.Unmanaged.slow 512 588 +14.8% 0.87x
UTF8Decode_InitFromBytes_ascii 292 323 +10.6% 0.90x (?)
UTF8Decode_InitFromData_ascii 263 287 +9.1% 0.92x (?)

Code size: -swiftlibs

How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 16 GB

@atrick
Copy link
Contributor

atrick commented Aug 3, 2021

Per discussion, the minimal fix would be as follows:

  • Both @in and @owned should be considered "consuming", thus mayrelease, regardless of the annotation (because the compiler can theoretically change conventions and also we haven't expected the library auther to take responsibililty for this)
  • @out should be considered a write regardless of the annotation (because it is compiler-generated... the library author can't know about it)

We should not fix, but should comment that

  • @inout requires the library author to know that the argument must be a trivial type.... the compiler will not check for them! In the future, we could have the compiler check, but for now we give maximum freedom to the library author.

@meg-gupta
Copy link
Contributor Author

meg-gupta commented Jul 15, 2025

Sources have changed considerably since I last looked at this. Documenting the issue in rdar://155870190 (SideEffects analysis can block optimizations in the presence of @effects) for now.

@meg-gupta meg-gupta closed this Jul 15, 2025
aidan-hall pushed a commit to aidan-hall/swift that referenced this pull request Aug 12, 2025
… attribute.

For the bug this patch aims to fix, we only need to compute the isDeinitBarrier
property. We could attempt to only store the computed value of the
isDeinitBarrier property, but there is no benefit to this over storing all the
computed values. The benefit of the previous behaviour was avoiding the overhead
of computing any properties in the first place.

rdar://155870190

This aims to resolve the same problem as swiftlang#38324.
aidan-hall pushed a commit to aidan-hall/swift that referenced this pull request Aug 12, 2025
… attribute.

For the bug this patch aims to fix, we only need to compute the isDeinitBarrier
property. We could attempt to only store the computed value of the
isDeinitBarrier property, but there is no benefit to this over storing all the
computed values. The benefit of the previous behaviour was avoiding the overhead
of computing any properties in the first place.

rdar://155870190

This aims to resolve the same problem as swiftlang#38324.
aidan-hall pushed a commit to aidan-hall/swift that referenced this pull request Aug 15, 2025
… attribute.

For the bug this patch aims to fix, we only need to compute the isDeinitBarrier
property. We could attempt to only store the computed value of the
isDeinitBarrier property, but there is no benefit to this over storing all the
computed values. The benefit of the previous behaviour was avoiding the overhead
of computing any properties in the first place.

rdar://155870190

This aims to resolve the same problem as swiftlang#38324.
aidan-hall pushed a commit to aidan-hall/swift that referenced this pull request Aug 19, 2025
… attribute.

This allows us to compute the deinit_barrier effect for such functions, fixing
the test case here: rdar://155870190

This supercedes swiftlang#38324.

ComputeSideEffects: Reformat tests to follow conventions

Includes moving irrelevant tests out of side_effects.sil

TempLValueElimination: deinit_barrier tests with effect attributes

ComputeSideEffects: Combine defined+computed effects optimistically

ComputeSideEffects: Store defined argumentEffects

This uses the same logic as Function.getSideEffects to determine the argument
effects of a function with an effectAttribute.

ComputeSideEffects: tests for defined argument effects

ComputeSideEffects: test defined effects overriding computed effects
aidan-hall pushed a commit to aidan-hall/swift that referenced this pull request Aug 20, 2025
… attribute.

This allows us to compute the deinit_barrier effect for such functions, fixing
the test case here: rdar://155870190

This supercedes swiftlang#38324.

ComputeSideEffects: Reformat tests to follow conventions

Includes moving irrelevant tests out of side_effects.sil

TempLValueElimination: deinit_barrier tests with effect attributes

ComputeSideEffects: Combine defined+computed effects optimistically

ComputeSideEffects: Store defined argumentEffects

This uses the same logic as Function.getSideEffects to determine the argument
effects of a function with an effectAttribute.

ComputeSideEffects: tests for defined argument effects

ComputeSideEffects: test defined effects overriding computed effects
aidan-hall pushed a commit to aidan-hall/swift that referenced this pull request Aug 21, 2025
… attribute.

This allows us to compute the deinit_barrier effect for such functions, fixing
the test case here: rdar://155870190

This supercedes swiftlang#38324.
aidan-hall pushed a commit to aidan-hall/swift that referenced this pull request Aug 22, 2025
After computing side effects, we also remove any global or argument effects that
are computed to happen, but are defined not to, based on the effect attribute.

This allows us to compute the deinit_barrier effect for such functions, fixing
the test case here: rdar://155870190

This supercedes swiftlang#38324.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants