Skip to content

[OSSACanonicalizeOwned] Record traversed defs and don't traverse copies of guaranteed values. #77968

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

Merged
merged 8 commits into from
Dec 6, 2024

Conversation

nate-chandler
Copy link
Contributor

@nate-chandler nate-chandler commented Dec 5, 2024

The utility performs two def-use traversals. The first determines liveness from uses. The second rewrites copies.

Previously, the defs whose uses are analyzed were discovered twice, once during each traversal. The non-triviality of the discovery logic (i.e. the logic determining when to walk into the values produced by the instructions which were the users of visited uses) opened the possibility for a divergence between the two discoveries. This possibility had indeed been realized--the two discoveries didn't find exactly the same defs, the two traversals didn't visit exactly the same uses, and issues ensued.

Here, the defs whose uses are analyzed are discovered only once during the first traversal. The second "traversal" now reuses the defs discovered in the first traversal, eliminating the possibility of a def discovery difference.

Extra care is taken to preserve the traversal order of the first traversal (as compared to the order before this change). Evidently diagnostics depend on it. The second traversal's order is changed, which results in slight SIL perturbations in certain cases (e.g. an instruction which consumes a value and its copy may see the order of those operands reversed).

Because discovery of defs walks into reborrows and borrowed-from instructions, copies are processed whose underlying value is a reborrow or borrowed-from. Such copies cannot generally be sunk to consuming uses because the lifetime of their underlying value (the reborrow or borrowed-from) may not extend to the consuming use. Eliminating such copies is outside the scope of OSSACanonicalizeOwned--it's the responsibility of a separate OSSACanonicalizeGuaranteed utility. For this reason, when visiting copies during the first traversal, the copies are only walked into (and added to the list of defs) if their operand is not a reborrow or borrowed-from.

rdar://139842132

Add a type which distinguishes among the types of defs that are pushed
onto the "def-use worklist".  Note that it's not possible to rely on the
kind of value because the root may itself be a copy_value.  For now, the
distinction is discarded as soon as the def is visited.
When rewriting uses, determine how that rewriting should proceed based
on the kind of def whose use is being visited.  Direct uses of reborrows
and borrowed-froms never need to be rewritten because every such use is
a use of a guaranteed value and can never require a copy.
Add a local variable that shadows the `defUseWorklist` instance member,
enabling further constraints (e.g. `const`ness) to eventually be imposed.
In preparation for only recording the defs once, replace the
GraphNodeWorklist of defs with a SetVector.  Preserve the current
visitation order by creating a worklist of indices to be visited.
The field is no longer a worklist, just a list of discovered defs.
@nate-chandler nate-chandler changed the title [OSSACanonicalizeOwned] Record traversed defs and require a copy for a final consume of a guaranteed original. [OSSACanonicalizeOwned] Record traversed defs and don't traverse copies of guaranteed values. Dec 5, 2024
The utility performs two def-use traversals.  The first determines
liveness from uses.  The second rewrites copies.

Previously, the defs whose uses were analyzed were discovered twice,
once during each traversal.  The non-triviality of the discovery logic
(i.e. the logic determining when to walk into the values produced by the
instructions which were the users of visited uses) opened the
possibility for a divergence between the two discoveries.  This
possibility had indeed been realized--the two traversals didn't visit
exactly the same uses, and issues ensue.

Here, the defs whose uses are analyzed are discovered only once (and not
discarded as their uses are analyzed) during the first traversal.  The
second traversal reuses the defs discovered in the first traversal,
eliminating the possibility of a def discovery difference.

The second traversal is now done in a different order.  This results in
perturbing the SIL in certain cases.
Because discovery of defs walks into reborrows and borrowed-from
instructions, copies may be seen whose underlying value is a guaranteed
value (namely, a reborrow or a borrowed-from instruction). Such copies
may be used beyond the lifetime end of such guaranteed values, so it's
not allowed to sink copies to their consuming uses. Such
canonicalization is the responsibility of the OSSACanonicalizeGuaranteed
utility.

rdar://139842132
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler
Copy link
Contributor Author

@swift-ci please test source compatibility

@nate-chandler
Copy link
Contributor Author

@swift-ci please apple silicon benchmark

@nate-chandler nate-chandler marked this pull request as ready for review December 5, 2024 20:07
@nate-chandler
Copy link
Contributor Author

@swift-ci please clean test linux platform

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nate-chandler
Copy link
Contributor Author

@swift-ci please test linux platform

@nate-chandler
Copy link
Contributor Author

@swift-ci please test macos platform

@nate-chandler nate-chandler merged commit 5637eaf into swiftlang:main Dec 6, 2024
8 checks passed
@nate-chandler nate-chandler deleted the rdar139842132 branch December 6, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants