-
Notifications
You must be signed in to change notification settings - Fork 10.5k
TempRValueOpt: extend access scopes #36540
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
Conversation
@swift-ci test |
@swift-ci benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -OnoneCode size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
Build failed |
Build failed |
Try to move an end_access down to extend the access scope over all uses of the temporary. For example: %a = begin_access %src copy_addr %a to [initialization] %temp : $*T end_access %a use %temp We must not replace %temp with %a after the end_access. Instead we try to move the end_access after "use %temp". This fixes generation of invalid SIL and/or the invalid removal of access checks.
bafc128
to
7c454e3
Compare
@swift-ci test |
@swift-ci benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks safe enough. Thanks!
@swift-ci smoke test linux |
} else if (endAccessToMove) { | ||
// We cannot move an end_access over a begin_access. This would destroy | ||
// the proper nesting of accesses. | ||
if (isa<BeginAccessInst>(&inst)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eeckstein We also still have an "unpaired" begin access, so strictly speaking, finding all the access scope beginnings means doing this:
isa<BeginAccessInst>(inst) || isa<BeginUnpairedAccessInst>(inst)
I'm surprised there's no helper for it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The begin_unpaired_access is only used in the KeyPath runtime functions. And we compile the stdlib without access enforcement. So it should not be a problem.
But checking for the unpaired access does not harm. I'll add it.
Try to move an
end_access
down to extend the access scope over all uses of the temporary.For example:
We must not replace
%temp
with%a
after the end_access. Instead we try to move theend_access
afteruse %temp
.This fixes generation of invalid SIL and/or the invalid removal of access checks.
The change also contains some small non-functional re-factorings/improvements.