Skip to content
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

Jump to bottom

[SR-5403] Memory Optimization Opportunity (Load/Store forwarding) #47977

Closed
aschwaighofer opened this issue Jul 7, 2017 · 8 comments
Closed

[SR-5403] Memory Optimization Opportunity (Load/Store forwarding) #47977

aschwaighofer opened this issue Jul 7, 2017 · 8 comments
Assignees
@weissi
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself performance

Comments

@aschwaighofer
Copy link
Member

Previous ID SR-5403
Radar rdar://problem/33187188
Original Reporter @aschwaighofer
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug, Performance
Assignee @weissi
Priority Medium

md5: df262c6b3bd0edc993b46ca475f49ed6

Issue Description:

This is sad:

// protocol witness for B.foo(_:) in conformance OtherB
sil shared [transparent] [serialized] [thunk] @_T04test6OtherBCAA1BA2aDP3fooyAA1ACFTW : $@convention(witness_method) (@owned A, @in_guaranteed OtherB) -> () {
// %0                                             // user: %7
// %1                                             // user: %3
bb0(%0 : $A, %1 : $*OtherB):
  %2 = alloc_stack $OtherB                        // users: %9, %4, %11, %7
  %3 = load %1 : $*OtherB                         // users: %6, %4
  store %3 to %2 : $*OtherB                       // id: %4
  // function_ref B.foo(_:)
  %5 = function_ref @_T04test1BPAAE3fooyAA1ACF : $@convention(method) <τ_0_0 where τ_0_0 : B> (@owned A, @in_guaranteed τ_0_0) -> () // user: %7
  strong_retain %3 : $OtherB                      // id: %6
  %7 = apply %5<OtherB>(%0, %2) : $@convention(method) <τ_0_0 where τ_0_0 : B> (@owned A, @in_guaranteed τ_0_0) -> ()
  %8 = tuple ()                                   // user: %12
  %9 = load %2 : $*OtherB                         // user: %10
  strong_release %9 : $OtherB                     // id: %10
  dealloc_stack %2 : $*OtherB                     // id: %11
  return %8 : $()                                 // id: %12
} // end sil function '_T04test6OtherBCAA1BA2aDP3fooyAA1ACFTW'

This should get fixed by moving to opaque value types. At that point the load store stuff would disappear and the ARC optimizer would see a retain/release on the same value and should remove it.

If memory ops could just tell that the apply does not write to the alloc_stack (it could because @in_guaranteed doe not modify the alloc_stack)_ … i would expect it to mem promote the store load sequence.
@aschwaighofer
Copy link
Member Author

@swift-ci create

@gottesmm
Copy link
Member

gottesmm commented Jul 7, 2017

I think that this could potentially be a starter bug. Might need an intermediate contributor maybe. Definitely on the easier spectrum of things IMO.

@gottesmm
Copy link
Member

gottesmm commented Jul 8, 2017

@weissi If you want, I can try to guide you towards a solution here. I am interested in helping more people understand how to make changes like this to the optimizer. So if you are interested, I am happy to help/guide you.

@weissi
Copy link
Member

weissi commented Jul 8, 2017

Hi @gottesmm, thanks for asking, that's actually a great idea. I'd be very interested on getting started to understand the optimiser a little more and even better if I can fix some bugs. We're working on some pretty performance sensitive stuff and there's quite a few cases where I was surprised. And so far it's quite an investment for me as I'm trying to work back from SIL/assembly and compare it to my expectations of what it should look like. But I reckon having an idea what's actually going on will help. Thanks so much for offering your help, looking forward to getting started 🙂. Any pointers very welcome!

@gottesmm
Copy link
Member

@weissi The first thing to do here is prepare a before/after test case. Specifically, I would take the test case that Arnold provided and try to strip out the irrelevant parts (i.e. witness tables, etc). To my mind, this means creating a simple test that performs a store, then passes the address to an in_guaranteed function and then reloads the value. In that case, we should be able to perform store to load forwarding. Then I would look into why redundant load eliminator is not forwarding the value. You can understand that by using sil-opt and enabling debug output from that specific pass. This is done by passing in an argument to the Debug Only LLVM option:

-debug-only=sil-redundant-load-elim

This is using a technique from llvm to conditional turn on/off debug output. The string is set at the top of the file that defines the optimization by the #define of DEBUG_FLAG. This output should say that it failed to perform the store->load forwarding for X reason. I am assuming that this is due to an analysis that is used by redundant load optimization. I believe the analysis that is failing is called MemoryBehavior. I would go in the debugger using sil-opt to see why it is failing. I believe the reason it is failing is b/c no one implemented the in_guaranteed optimization, but please confirm that. Once you have confirmed that, you should change the Memory Behavior analysis to be smarter about in_guaranteed and add a test to the redundant load elimination test and a test to the memory behavior dumper test. This will enable us to verify that:

1. The store to load forwarding is actually happening.
2. The memory behavior analysis is returning the correct values and that we are not performing the store to load forwarding due to some fluke in the analysis.

Hows that sound? Any questions?

@weissi
Copy link
Member

weissi commented Jul 10, 2017

@gottesmm thanks, that sounds great, following up via email.

@weissi
Copy link
Member

weissi commented Jul 18, 2017

PR: #11040

@weissi
Copy link
Member

weissi commented Jul 26, 2017

merged

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@weissi weissi

Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself performance
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gottesmm @weissi @aschwaighofer