-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[semantic-arc] Attempt to handle indirect_mutating parameters that are never written to. #29480
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
[semantic-arc] Attempt to handle indirect_mutating parameters that are never written to. #29480
Conversation
@swift-ci smoke test |
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.
To handle calls, this can be converted into a bottom-up IPA... eventually...
Also, is there a possibility this will be called on the same argument multiple times? It's a red flag that you're calling an unbounded def-use analysis from within a use-def analysis.
} | ||
|
||
// load_borrow and debug_value_addr are both fine as well. | ||
if (isa<LoadBorrowInst>(user) || isa<DebugValueAddrInst>(user)) { |
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.
When visiting uses, I like to use isIncidentalUse
, instead of checking for Debug opcodes so we can add different "marker" instructions later without auditing all passes.
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.
Sure.
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.
Yes. I was just writing something up real quickly. Also I realized I need to look through access markers.
// interprocedural analysis that we do not perform here. | ||
if (auto fas = FullApplySite::isa(user)) { | ||
if (fas.getArgumentConvention(*op) == | ||
SILArgumentConvention::Indirect_In_Guaranteed) |
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.
Too bad you need to hard-code Full-Apply + Indirect_In_Guaranteed
. I feel like we should be using the isMutatingOrConsuming
logic from the verifier to handle closures, coroutines, and other conventions... but it looks like it returns an overly optimistic results for inout_aliasable
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.
I was just trying to do something really simple. I am going to refactor it out.
@atrick I don't think this will be unbounded since it does not trigger the parent use-def walk in any way. We may perform the walk multiple times, but always a bounded amount of times. |
From talking with Andy, he was actually referring to quadratic behavior. |
When I say "unbounded" I mean there's nothing stopping it from running away on pathological code. You could have 1000 use-def walks in a pathological case, which is ok, but then then same def-use chain will be visited again.. quadratically |
40a7730
to
ac486a1
Compare
@swift-ci smoke test |
1 similar comment
@swift-ci smoke test |
@swift-ci benchmark |
ac486a1
to
79b77f2
Compare
Build failed before running benchmark. |
@swift-ci test |
@swift-ci smoke test |
Build failed |
Build failed |
79b77f2
to
0f924f0
Compare
Going to do a little more work on this real quick in the background and make sure at least when building the stdlib, I am not missing anything. |
8da4eed
to
a4bf2c1
Compare
@swift-ci smoke test |
@swift-ci benchmark |
1 similar comment
@swift-ci benchmark |
@swift-ci benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -Osize
Performance: -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
|
I am going to clean up this a bit to get rid of the quadratic behavior and then in a follow on commit, I am going to add support for handling more complex cases including cases where the value is written to but not in the region we are loading within. I have a simple implementation that should work. |
…e never written to. It only handles local writes, so we can not handle chains of inout parameters. That being said, I wonder if we could hook this up to an analysis to determine if the inout has this same property in callees.
a4bf2c1
to
f9b0f99
Compare
@swift-ci test |
@swift-ci test source compatibility |
@swift-ci benchmark |
@swift-ci test |
@swift-ci test windows platform |
@swift-ci benchmark |
@swift-ci test source compatibility |
@swift-ci test windows platform |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -Osize
Performance: -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
|
It only handles local writes, so we can not handle chains of inout parameters.
That being said, I wonder if we could hook this up to an analysis to determine
if the inout has this same property in callees.
rdar://problem/58667192
Just had to get this out of my fingers.