Skip to content

Use AccessPath in LICM and split loads for combine load/store hoisting #33987

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.

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

Merged
merged 3 commits into from
Nov 13, 2020

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Sep 18, 2020

Still waiting to merge #33121
And creating more thorough test cases for this.

Fixes regressions caused by rdar://66791257 (Print statement provokes "Can't unsafeBitCast between types of different sizes" when optimizations enabled)

Step 1:
Use AccessPath in LICM.

The LICM algorithm was not robust with respect to address projection
because it identifies a projected address by its SILValue. This should
never be done! Use AccessPath instead.

Step 2:
LICM: split loads that are wider than the loop-stored value.

For combined load-store hoisting, split loads that contain the
loop-stored value into a single load from the same address as the
loop-stores, and a set of loads disjoint from the loop-stores. The
single load will be hoisted while sinking the stores to the same
address. The disjoint loads will be hoisted normally in a subsequent
iteration on the same loop.

loop:
  load %outer
  store %inner1
exit:

Will be split into

loop:
  load %inner1
  load %inner2
  store %inner1
exit:

Then, combined load/store hoisting will produce:

  load %inner1
loop:
  load %inner2
exit:
  store %inner1

@atrick atrick changed the title [WIP] Opt licm combined ldst [WIP] Use AccessPath in LICM and split loads for combine load/store hoisting Sep 18, 2020
@atrick atrick requested a review from eeckstein September 18, 2020 01:23
@atrick atrick force-pushed the opt-licm-combined-ldst branch from e4e82d5 to ee0ce56 Compare September 23, 2020 00:45
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.

First step for my review: I'd like to fully understand how this should work. So I just reviewed the documentation for now. Once I have a clear picture, I continue with the implementation.

@atrick atrick force-pushed the opt-licm-combined-ldst branch from ee0ce56 to 715455f Compare September 24, 2020 04:38
@atrick
Copy link
Contributor Author

atrick commented Sep 29, 2020

@eeckstein documentation feedback was all very helpful. The rewritten documentation is here:

https://github.com/apple/swift/blob/d5590801e44b51f96c371dc8d81e619687ae71b0/docs/SILProgrammersManual.md#accessedstorage-and-accesspath

Keep in mind, I'm keeping it as concise as I can since I think details belong in file or class level comments.

This PR is only supposed to cover the LICM load splitting optimization.

@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)

@atrick atrick changed the base branch from master to main October 2, 2020 03:03
The LICM algorithm was not robust with respect to address projection
because it identifies a projected address by its SILValue. This should
never be done! Use AccessPath instead.

Fixes regressions caused by rdar://66791257 (Print statement provokes
"Can't unsafeBitCast between types of different sizes" when
optimizations enabled)
@atrick atrick force-pushed the opt-licm-combined-ldst branch from 715455f to c374029 Compare November 10, 2020 21:27
@atrick atrick changed the title [WIP] Use AccessPath in LICM and split loads for combine load/store hoisting Use AccessPath in LICM and split loads for combine load/store hoisting Nov 10, 2020
@atrick
Copy link
Contributor Author

atrick commented Nov 10, 2020

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Nov 10, 2020

@swift-ci test source compatibility

@atrick
Copy link
Contributor Author

atrick commented Nov 10, 2020

@swift-ci benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Regression OLD NEW DELTA RATIO
StringAdder 224 241 +7.6% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
Data.hash.Medium 31 27 -12.9% 1.15x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
FlattenListLoop 930 1048 +12.7% 0.89x (?)
 
Improvement OLD NEW DELTA RATIO
Chars2 3700 3300 -10.8% 1.12x (?)
Calculator 166 149 -10.2% 1.11x (?)
Data.hash.Medium 31 28 -9.7% 1.11x
ObjectiveCBridgeStubToNSStringRef 83 75 -9.6% 1.11x (?)

Code size: -Osize

Performance: -Onone

Regression OLD NEW DELTA RATIO
ArrayAppendAsciiSubstring 28656 31176 +8.8% 0.92x (?)
DataToStringSmall 3600 3900 +8.3% 0.92x (?)
ArrayAppendLatin1Substring 29304 31716 +8.2% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
Data.hash.Medium 40 37 -7.5% 1.08x (?)
LuhnAlgoEager 2525 2349 -7.0% 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 mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - c37402993f3b6ede773ac016fdb865b2ef041648

@atrick atrick force-pushed the opt-licm-combined-ldst branch from c374029 to dc660c5 Compare November 12, 2020 05:22
For combined load-store hoisting, split loads that contain the
loop-stored value into a single load from the same address as the
loop-stores, and a set of loads disjoint from the loop-stores. The
single load will be hoisted while sinking the stores to the same
address. The disjoint loads will be hoisted normally in a subsequent
iteration on the same loop.

loop:
  load %outer
  store %inner1
exit:

Will be split into

loop:
  load %inner1
  load %inner2
  store %inner1
exit:

Then, combined load/store hoisting will produce:

  load %inner1
loop:
  load %inner2
exit:
  store %inner1
@atrick atrick force-pushed the opt-licm-combined-ldst branch from dc660c5 to 437765e Compare November 12, 2020 22:00
@atrick
Copy link
Contributor Author

atrick commented Nov 12, 2020

I finished writing tests for corner cases and caught a potential miscompile.

@atrick
Copy link
Contributor Author

atrick commented Nov 12, 2020

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Nov 12, 2020

@swift-ci test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 437765e

@atrick
Copy link
Contributor Author

atrick commented Nov 13, 2020

I'm seeing a PR testing issue:
Failed test:
lldb-api lang/swift/unknown_self/TestSwiftUnknownSelf.py

libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: recursive_mutex lock failed: Invalid argument

@atrick
Copy link
Contributor Author

atrick commented Nov 13, 2020

And another PR testing failure with the Debug SCK

<unknown>:0: error: fatal error encountered during compilation; please submit a bug report (https://swift.org/contributing/#reporting-bugs) and include the project
15:01:47 <unknown>:0: note: Cannot read '/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/build/compat_macos/swift-macosx-x86_64/./lib/swift/macosx/layouts-x86_64.yaml'

@atrick
Copy link
Contributor Author

atrick commented Nov 13, 2020

@swift-ci smoke test OS X

@atrick atrick merged commit 4409f14 into swiftlang:main Nov 13, 2020
@compnerd
Copy link
Member

This seems to have failed to build on Windows, can we revert this?

compnerd added a commit to compnerd/apple-swift that referenced this pull request Nov 13, 2020
@atrick atrick deleted the opt-licm-combined-ldst branch October 19, 2022 00:31
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.

6 participants