-
Notifications
You must be signed in to change notification settings - Fork 10.5k
SILGen: Ease off +0 peepholes for load exprs. #11026
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
SILGen: Ease off +0 peepholes for load exprs. #11026
Conversation
@swift-ci Please smoke test |
@swift-ci Please test source compatibility |
@rjmccall @devincoughlin @gottesmm Does this look right? I'm not fully familiar with the changes that went into lvalue emission for exclusivity checking and ownership enforcement, but something changed between 3.1 and now that's making these +0 peepholes break, since the temporary buffers for computed component projections get destroyed apparently earlier than they used to. |
@swift-ci Please benchmark |
I think you're probably right that it's just not valid to do this without some more concrete idea of how long we're within the borrow. And yeah, the access will end immediately. |
Build comment file:Optimized (O)Regression (37)
Improvement (13)
No Changes (271)
Unoptimized (Onone)Regression (154)
Improvement (1)
No Changes (166)
Hardware Overview
|
@gottesmm @aschwaighofer The effect on benchmarks looks pretty severe. Is there any minor improvement we could make to the ARC optimizer to get rid of the extra retain in situations when it's not needed, when it would be safe to widen the access into a lvalue? |
@swift-ci Please benchmark |
There aren't very many places where we actually use AllowImmediatePlusZero emission in SILGen, and most of them are pretty random like x.dynamicType; I'm surprised it has a significant impact. In general it's architecturally bogus and we should instead be passing down a closure (staged into an Initialization, I guess) to use the value within the access. I don't think you actually need to change the behavior for AllowGuaranteedPlusZero emission. On the other hand, loads shouldn't be taking advantage of that unless there's something sneaking in that's not really a load. |
True, |
0e8af5f
to
82d2a81
Compare
@swift-ci Please smoke test |
@swift-ci Please benchmark |
@rjmccall How's this look now? |
Now that we more tightly close formal accesses on lvalues, having LoadExpr and friends try to return a +0 loaded value is unsafe without deeper analysis, since the access will be closed immediately after the load and potentially free temporary memory that might be the only thing keeping the borrowed object alive. Fixes rdar://problem/32730865.
82d2a81
to
7705393
Compare
@swift-ci Please smoke test |
@swift-ci Please benchmark |
Looks great, thanks. Definitely happier. |
Build comment file:Optimized (O)Regression (36)
Improvement (7)
No Changes (278)
Unoptimized (Onone)Regression (163)
Improvement (1)
No Changes (157)
Hardware Overview
|
Hmm, there does seem to be a consistent impact on benchmarks. I filed https://bugs.swift.org/browse/SR-5508 to investigate. @bob-wilson, is it OK to take this correctness fix now? |
Yes, we can't sacrifice correctness for performance. I'm OK to go ahead with this. |
on Wed Jul 19 2017, Bob Wilson <notifications-AT-i.8713187.xyz> wrote:
Yes, we can't sacrifice correctness for performance. I'm OK to go ahead with this.
Don't we at least need to have a plan for recovering the losses?
…--
-Dave
|
We're investigating the performance issues as a top-priority. |
Now that we more tightly close formal accesses on lvalues, having LoadExpr and friends try to return a +0 loaded value is unsafe without deeper analysis, since the access will be closed immediately after the load and potentially free temporary memory that might be the only thing keeping the borrowed object alive. Fixes rdar://problem/32730865.