-
Notifications
You must be signed in to change notification settings - Fork 439
Dyno: make VarScopeVisitor analyses work per-tuple element
#27563
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
Open
arifthpe
wants to merge
93
commits into
chapel-lang:main
Choose a base branch
from
arifthpe:tuple-unpacking-copy-init
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
init= for tuple destructuring init/assignment
ae73f1c to
2f842ef
Compare
2 tasks
b672363 to
469ca59
Compare
25d6f9f to
5865bae
Compare
init= for tuple destructuring init/assignment9733f63 to
77d8bb2
Compare
Signed-off-by: Anna Rift <[email protected]>
Signed-off-by: Anna Rift <[email protected]>
df75778 to
669673c
Compare
Signed-off-by: Anna Rift <[email protected]>
Signed-off-by: Anna Rift <[email protected]>
Signed-off-by: Anna Rift <[email protected]>
Signed-off-by: Anna Rift <[email protected]>
2055783 to
08f920b
Compare
Signed-off-by: Anna Rift <[email protected]>
Signed-off-by: Anna Rift <[email protected]>
df5952f to
8ec2abf
Compare
Signed-off-by: Anna Rift <[email protected]>
2078f34 to
b452107
Compare
Signed-off-by: Anna Rift <[email protected]>
Signed-off-by: Anna Rift <[email protected]>
a4271cd to
0c1fe70
Compare
Signed-off-by: Anna Rift <[email protected]>
4446f29 to
b1be0d8
Compare
Signed-off-by: Anna Rift <[email protected]>
VarScopeVisitor analyses aware of tuple components
VarScopeVisitor analyses aware of tuple componentsVarScopeVisitor analyses work per-tuple element
Contributor
|
Yay, marked non-draft! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjust
VarScopeVisitorand the analyses implemented using it (call-init-deinit, copy elision, split init) to operate per-tuple element, as appropriate.Explanation of approach
The main strategy here is to treat each tuple component within a tuple destructuring decl/assign as though it was a decl/assign of an individual variable, e.g.,
var (a, (b,)) : (int, (real,)) = (1, (2.0,));will be treated similarly tovar a : int = 1; var b : real = 2.0;. VSV is adjusted to gather information on tuple decls/assigns as it goes (including nested tuples), but not holistically process them as decls/assigns. That processing only happens once each individualVarLikeDecl/Identifiercontained within is reached, using info propagated from surrounding tuples to provide a pseudo- init-part and type-part as though it were a standalone decl/assign. As part of this approach, decl/assign handlers in VSV subclasses are refactored to accept a separate LHS and RHS rather than an actual decl/assign AST node, so they are agnostic to actual standalone decl/assigns vs the pseudo ones presented for destructured tuple elements.There are a few changes to the output of VSV analyses to accommodate tuples:
With this change,
AssociatedActions are now generated for each tuple element as appropriate, and can be different for each element. It also includes additions toAssociatedActionitself, to accommodate cases where there is an Action on the tuple as a whole as well as its components:An example where the above changes are relevant is the following:
Here we would record an
INIT_OTHERfortupwhich contained anASSIGNandMOVE_INITsub-action for its elements, respectively. Then we would have aCOPY_INITforxwhich contained anASSIGNandCOPY_INITfor its elements, respectively, and since there is no RHS AST to store the IDs of in the sub-actions to keep track of which action belongs to which element, we would store the element indices.A major drawback of this approach is that it does not elegantly handle the case of a (non-destructuring) tuple variable LHS and a tuple expression RHS, like
var myTuple = (a, b);. The VSV changes are centered around traversing the LHS and creating an equivalent decl/assign for each LHS component, but in this case there is no AST to traverse that represents the tuple elements. However, some VSV analyses still need to handle a tuple expr RHS on a per-component basis, such as noting variable mentions and finding elided copies. This PR does so by adding special behavior to each of the VSV subclasses which need to do such handling. Some alternatives considered were:init=/=call on the tuple which would perform per-element initialization/assignment. This should give us handling of copy elision etc for free, as each RHS component would be an argument to the call and processed as those normally are. I'm not sure if this approach could also cover the destructuring decl/assign case well. I didn't look into this route because I didn't think of it until Ben suggested it when I asked for input, by which time I had already nearly finished the VSV subclass special behavior approach.This PR also reverts the change from #27533 which changed tuple expressions containing any refs from
CONST_VARtoCONST_REFkind, as discussed from #27533 (comment).Contributes to https://github.com/Cray/chapel-private/issues/6283.
Future work:
DEINITfor tuples (part of https://github.com/Cray/chapel-private/issues/6283). Not included due to the size of this PR.Note to reviewer: The commit history of this branch is a mess due to abandoning an earlier approach, so it'd be best to review the diff as a whole.
[reviewer info placeholder]
Testing: