Skip to content

[LVA] Don't invalidate reads/writes that don't alias any tracked values. #31136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[LVA] Don't invalidate reads/writes that don't alias any tracked values. #31136

wants to merge 2 commits into from

Conversation

zoecarver
Copy link
Contributor

In DSE and RLE any unknown read/write often causes most if not all tracked values to be invalidated. This doesn't need to be the case. If the tracked value doesn't alias any of the operands of the unknown instruction, then we can safely keep the tracked value validated.

Requires #31131

@zoecarver zoecarver mentioned this pull request Apr 19, 2020
Closed
@zoecarver
[LVA] Don't invalidate reads/writes that don't alias any tracked values.
313c130 
In DSE and RLE any unkown read/write often causes most if not all tracked values to be invalidated. This doesn't need to be the case. If the tracked value doesn't alias any of the operands of the unkown instruction, then we can safely keep the tracked value validated.
@zoecarver zoecarver force-pushed the lva/dont-invalidate-no-alias branch from 092f116 to 313c130 Compare April 19, 2020 23:06
eeckstein
eeckstein reviewed Apr 21, 2020
View reviewed changes
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

I don't think you can make this assumption.
Let's assume an instruction (which could e.g. be a function call), which has an address of an object reference as operand and reads that reference and writes some field in the object.
The operand (address of the reference) is not aliased with the object itself, but would overwrite values in the object.

@zoecarver zoecarver mentioned this pull request Apr 21, 2020
Closed
@zoecarver
Copy link
Contributor Author

In that case, the operand of the instruction (function call) will alias with the pointer. So, that pointer's not being tracked anymore. I assume you're talking about promoting a load of the pointer so RLE would check if the operand (the pointer) is being tracked, it's not, so we wouldn't promote it.

We do introduce the problem #31131 fixes.

@zoecarver
Copy link
Contributor Author

I'm going to benchmark #31134 and #31132 together and compare it to the benchmark results from #31078. That will tell us if this patch is a significant performance improvement. If it's not, then I'm happy to drop it.

@zoecarver zoecarver mentioned this pull request Apr 22, 2020
Closed
@zoecarver
Copy link
Contributor Author

Given my comment on the performance improvements of this patch. I think it would be beneficial to move forward with this change.

@eeckstein sorry for all the messages recently. No rush responding.

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Regression OLD NEW DELTA RATIO
ObjectiveCBridgeFromNSSetAnyObjectToStringForced 90500 98500 +8.8% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
SortIntPyramid 1305 640 -51.0% 2.04x
ObjectiveCBridgeStubDataAppend 5420 3420 -36.9% 1.58x
SortAdjacentIntPyramids 1565 1180 -24.6% 1.33x
Data.append.Sequence.64kB.Count0.RE 350 291 -16.9% 1.20x
ObjectiveCBridgeStubFromNSDateRef 5720 4760 -16.8% 1.20x (?)
Data.append.Sequence.64kB.Count0.RE.I 349 291 -16.6% 1.20x (?)
ObjectiveCBridgeStubFromNSDate 7120 5980 -16.0% 1.19x (?)
Data.append.Sequence.64kB.Count0.I 343 289 -15.7% 1.19x (?)
Data.init.Sequence.64kB.Count0.RE.I 344 290 -15.7% 1.19x
Data.init.Sequence.64kB.Count0.I 344 290 -15.7% 1.19x
Data.init.Sequence.64kB.Count0 343 290 -15.5% 1.18x
Data.append.Sequence.64kB.Count0 344 291 -15.4% 1.18x
Data.init.Sequence.64kB.Count0.RE 344 291 -15.4% 1.18x
Data.append.Sequence.809B.Count0.RE.I 472 402 -14.8% 1.17x
Data.append.Sequence.809B.Count0.RE 472 402 -14.8% 1.17x
Data.append.Sequence.809B.Count0.I 467 400 -14.3% 1.17x
Data.append.Sequence.809B.Count0 466 400 -14.2% 1.16x
Data.init.Sequence.2047B.Count0.I 610 526 -13.8% 1.16x
Data.init.Sequence.2049B.Count0.I 610 526 -13.8% 1.16x
Data.init.Sequence.809B.Count0 536 466 -13.1% 1.15x
Data.init.Sequence.809B.Count0.RE 532 466 -12.4% 1.14x
Data.init.Sequence.809B.Count0.RE.I 531 466 -12.2% 1.14x (?)
Data.init.Sequence.809B.Count0.I 532 468 -12.0% 1.14x
Data.init.Sequence.511B.Count0.I 552 487 -11.8% 1.13x
Data.init.Sequence.513B.Count0.I 551 487 -11.6% 1.13x

Code size: -O

Regression OLD NEW DELTA RATIO
ObjectiveCBridgingStubs.o 16741 18165 +8.5% 0.92x
StringRemoveDupes.o 5573 5669 +1.7% 0.98x
 
Improvement OLD NEW DELTA RATIO
Prefix.o 18448 18024 -2.3% 1.02x
PrefixWhile.o 17734 17526 -1.2% 1.01x
DropWhile.o 17956 17748 -1.2% 1.01x

Performance: -Osize

Regression OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_insert 4858 5684 +17.0% 0.85x
DictionaryOfAnyHashableStrings_lookup 3936 4392 +11.6% 0.90x
DropWhileAnySeqCntRange 163 180 +10.4% 0.91x (?)
DropWhileAnySeqCRangeIter 180 198 +10.0% 0.91x (?)
FlattenListFlatMap 6843 7419 +8.4% 0.92x (?)
ObjectiveCBridgeStubToNSStringRef 116 125 +7.8% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
ObjectiveCBridgeStubDataAppend 5400 3820 -29.3% 1.41x
Data.append.Sequence.64kB.Count0.RE.I 357 301 -15.7% 1.19x
Data.append.Sequence.64kB.Count0.RE 357 301 -15.7% 1.19x
Data.init.Sequence.64kB.Count0.RE 358 302 -15.6% 1.19x
Data.append.Sequence.809B.Count0.RE.I 486 410 -15.6% 1.19x (?)
Data.init.Sequence.64kB.Count0.RE.I 357 302 -15.4% 1.18x
Data.append.Sequence.64kB.Count0.I 358 303 -15.4% 1.18x
Data.append.Sequence.64kB.Count0 359 304 -15.3% 1.18x (?)
Data.init.Sequence.64kB.Count0 358 304 -15.1% 1.18x
Data.init.Sequence.64kB.Count0.I 359 305 -15.0% 1.18x
Data.append.Sequence.809B.Count0 490 419 -14.5% 1.17x
Data.append.Sequence.809B.Count0.RE 485 416 -14.2% 1.17x
Data.append.Sequence.809B.Count0.I 489 420 -14.1% 1.16x (?)
Data.init.Sequence.2049B.Count0.I 635 551 -13.2% 1.15x (?)
Data.init.Sequence.2047B.Count0.I 634 551 -13.1% 1.15x
Data.init.Sequence.809B.Count0.RE 555 484 -12.8% 1.15x
Data.init.Sequence.809B.Count0.RE.I 553 483 -12.7% 1.14x (?)
Data.init.Sequence.809B.Count0 554 487 -12.1% 1.14x (?)
Data.init.Sequence.809B.Count0.I 555 489 -11.9% 1.13x
Data.init.Sequence.511B.Count0.I 573 508 -11.3% 1.13x (?)
Data.init.Sequence.513B.Count0.I 571 508 -11.0% 1.12x (?)
PrefixAnySeqCRangeIterLazy 176 159 -9.7% 1.11x
PrefixAnySeqCntRangeLazy 176 159 -9.7% 1.11x
PrefixWhileAnySeqCRangeIterLazy 176 159 -9.7% 1.11x (?)
CSVParsing.Scalar 218 198 -9.2% 1.10x (?)
StringRemoveDupes 413 380 -8.0% 1.09x (?)
RandomShuffleLCG2 880 816 -7.3% 1.08x
Array2D 8144 7568 -7.1% 1.08x (?)
ArrayPlusEqualFiveElementCollection 8954 8325 -7.0% 1.08x (?)
RemoveWhereSwapInts 72 67 -6.9% 1.07x (?)
SortAdjacentIntPyramids 1680 1570 -6.5% 1.07x (?)

Code size: -Osize

Regression OLD NEW DELTA RATIO
StringRemoveDupes.o 3895 4827 +23.9% 0.81x
 
Improvement OLD NEW DELTA RATIO
DropWhile.o 18084 17844 -1.3% 1.01x
PrefixWhile.o 16684 16508 -1.1% 1.01x

Performance: -Onone

Regression OLD NEW DELTA RATIO
StringBuilderWithLongSubstring 2730 3230 +18.3% 0.85x (?)
DictionaryOfAnyHashableStrings_insert 7028 8022 +14.1% 0.88x (?)
ObjectiveCBridgeFromNSSetAnyObjectToStringForced 88500 100500 +13.6% 0.88x (?)
UTF8Decode_InitFromData_ascii_as_ascii 792 876 +10.6% 0.90x (?)
Dictionary2 1210 1335 +10.3% 0.91x (?)
Breadcrumbs.MutatedUTF16ToIdx.ASCII 21 23 +9.5% 0.91x (?)
Breadcrumbs.MutatedIdxToUTF16.ASCII 21 23 +9.5% 0.91x (?)
DictionaryOfAnyHashableStrings_lookup 4584 5016 +9.4% 0.91x (?)
ArrayOfPOD 1037 1118 +7.8% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
ObjectiveCBridgeStubDataAppend 6040 4460 -26.2% 1.35x
Set.isDisjoint.Int0 1302 968 -25.7% 1.35x
SetSubtractingInt25 388 307 -20.9% 1.26x
SetSubtractingInt0 283 229 -19.1% 1.24x
SetSubtractingInt50 445 363 -18.4% 1.23x
SetSubtractingBox25 923 756 -18.1% 1.22x
SetSubtractingInt100 540 446 -17.4% 1.21x
SetUnionInt50 516 438 -15.1% 1.18x
Set.isDisjoint.Int25 1231 1047 -14.9% 1.18x
NSStringConversion.Mutable 2860 2439 -14.7% 1.17x (?)
Set.isDisjoint.Int50 1195 1020 -14.6% 1.17x
SetUnionInt100 346 297 -14.2% 1.16x
SetUnionInt25 584 502 -14.0% 1.16x
Set.isDisjoint.Box0 3112 2712 -12.9% 1.15x
SetSubtractingBox0 656 576 -12.2% 1.14x
SetUnion 9080 8030 -11.6% 1.13x (?)
SetUnionBox25 1429 1266 -11.4% 1.13x
SetUnionInt0 906 803 -11.4% 1.13x
NSStringConversion.UTF8 2357 2105 -10.7% 1.12x (?)
StringRemoveDupes 831 743 -10.6% 1.12x (?)
Set.isDisjoint.Int100 893 808 -9.5% 1.11x
DataCountMedium 100 91 -9.0% 1.10x (?)
StringFromLongWholeSubstring 12 11 -8.3% 1.09x (?)
Set.isDisjoint.Box25 2520 2312 -8.3% 1.09x (?)
Set.isDisjoint.Int.Empty 668 615 -7.9% 1.09x (?)
DictionaryCompactMapValuesOfCastValue 58644 54270 -7.5% 1.08x (?)
Dictionary4OfObjectsLegacy 1728 1605 -7.1% 1.08x (?)
SetUnion_OfObjects 25270 23510 -7.0% 1.07x (?)
SetUnionBox0 2526 2352 -6.9% 1.07x (?)

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: 64 GB

@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@zoecarver zoecarver closed this Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eeckstein eeckstein eeckstein left review comments

@slavapestov slavapestov Awaiting requested review from slavapestov

@atrick atrick Awaiting requested review from atrick

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zoecarver @swift-ci @shahmishal @eeckstein