Skip to content

[opt] Teach mem2reg about begin_access. #30812

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

Closed
wants to merge 1 commit into from

Conversation

zoecarver
Copy link
Contributor

Mem2reg can now follow, project, and remove being_access instructions. This will be particularly beneficial for #30810.

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark.

@zoecarver
Copy link
Contributor Author

I suspect that there won't be significant improvement because mem2reg only optimizes tuples and structs and those rarely have access markers around them or their members.

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Regression OLD NEW DELTA RATIO
FlattenListLoop 2874 3278 +14.1% 0.88x (?)
ObjectiveCBridgeStubNSDateRefAccess 530 571 +7.7% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
FlattenListFlatMap 6293 4630 -26.4% 1.36x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 3522 4921 +39.7% 0.72x (?)
FlattenListLoop 2869 3262 +13.7% 0.88x (?)

Code size: -Osize

Performance: -Onone

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

@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 changed the base branch from master to main October 2, 2020 04:21
@zoecarver
Copy link
Contributor Author

Now that #34276 has landed, I'm going to try benchmarking this again.

Mem2reg can now follow, project, and remove being_access instructions.
@zoecarver zoecarver force-pushed the opt/mem2reg-begin-access branch from 67cde92 to 1f00e9a Compare December 6, 2020 19:34
@zoecarver
Copy link
Contributor Author

At some point, it would probably also be good to re-write the patch to use the AccessPath utility.

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark.

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark.

@swift-ci
Copy link
Contributor

swift-ci commented Dec 6, 2020

Performance: -O

Regression OLD NEW DELTA RATIO
FlattenListLoop 1018 1543 +51.6% 0.66x (?)
DropLastArrayLazy 4 5 +25.0% 0.80x (?)
FlattenListFlatMap 4088 4446 +8.8% 0.92x (?)

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

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

@eeckstein
Copy link
Contributor

@zoecarver I'm wondering if mem2reg actually needs to handle access instructions. Wouldn't access marker elimination remove those markers anyway?
Did you see a swift code pattern where this change makes a difference?

@zoecarver
Copy link
Contributor Author

I'm surprised to see such a large regression. I wonder what's causing that.

@zoecarver I'm wondering if mem2reg actually needs to handle access instructions. Wouldn't access marker elimination remove those markers anyway?
Did you see a swift code pattern where this change makes a difference?

Yes, marker elimination should remove these. I thought that happened later in the pass pipeline than it does. It looks like it happens in the high-level function pipeline, though, so that should be sufficient (which explains why there aren't performance gains here).

I was looking at code patterns as simple as:

struct Foo { var x = 0 }

func test() -> Int {
    var f = Foo()
    f.x = 1
    return f.x
}

Because of how early the mem2reg pass runs in the pipeline, it looked like it was getting tripped up on the access markers.

@zoecarver zoecarver closed this Dec 8, 2020
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.

4 participants