Skip to content

[region-isolation] Fix the dataflow and add support for project_block_storage #70514

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

Conversation

gottesmm
Copy link
Contributor

In this PR, I fix three issues that came up when we were doing some testing:

  1. There was an issue with the way the dataflow was implemented in the initial prototype. Specifically, we only initially visited the first block instead of initially visiting all blocks. This could make it so we did not visit all of the blocks we needed to. I fixed this and standardized the way we performed the dataflow.
  2. Once I fixed that issue, I discovered a bug from the initial implemenation in the way we canonicalized in our union find-ish data structure. Specifically, we only updated the "fresh region" counter when the newest largest region was > that that value. We needed to also expand when it was equal.
  3. Finally while doing that last testing, we hit an issue where since project_block_storage was not supported, we crashed when handling APIs that take blocks (e.x.: dispatch.sync and friends).

I also renamed the tests with prefix sendnonsendable_* -> transfernonsendable_* given that I did the rename earlier. At the time I did that other rename in code I didn't do the change since I had a bunch of out of tree changes to those tests and didn't want to run into merge conflicts. That's all upstreamed now so we are good to go.

rdar://119584497
rdar://119743743

…rnonsendable_*

I also renamed sendnonsendable_basic.sil ->
transfernonsendable_instruction_matching.sil since that is what the test is
intended to be used for... mechanical instruction level modeling tests rather
than more complex crashers.
This involved fixing a few different bugs.

1. We were just performing dataflow by setting that only the initial block needs
to be updated. This means that if there isn’t anything in the initial dataflow
block, we won’t visit any successor blocks. Instead the correct thing to do here
is to visit all blocks in the initial round.

2. I also needed to fix a separate issue where we were updating our union-find
data structure incorrectly as found by an assert on transfernonsendable.swift
that was triggered once I fixed 1. Put simply, we needed to set a new max label
+ 1 when our new max element is less than or equal to the old max label + 1…
before we just did less than so if we had a new max element that is the same as
our fresh label, we wouldn’t increment the fresh label.

rdar://119584497
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

@gottesmm gottesmm force-pushed the pr-763529d8ee9f4164b82d03087fec25439cc1d315 branch from 41f051d to 7712639 Compare December 18, 2023 18:08
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

…in instructions that can only appear in non-OSSA as asserting.

We already only supported Ownership SSA since we run early in the pipeline
before OSSA is lowered. This just formalizes this behavior. I am marking these
instructions as Asserting (even though we will never see them) so I can
semantically be sure that all of the instructions are covered without using an
"unsupported" like moniker that I fear will lead to new instructions being added
as unsupported. Better to have a semantic thing for new instruction adders to
use.
fix_lifetime just acts as a require.
…adata since they only appear in Lowered SIL.

I also added a note to the SIL.rst and an assert into the SILVerifier to better
document this requirement.
…t as an assign from op 0 -> result and a require of op 1.

Semantically a mark_dependence returns a value that is equal to its first
parameter with the extra semantics that any destroys of the 2nd operand cannot
occur before any uses of the result of the instruction. From a region
perspective this suggests that the instruction should be an assign from the
first operand onto the result and act as a require on the result. Semantically
the requirement that the 2nd operand cannot be destroyed before any uses of the
result does not expose any memory or state from the first operand implying that
we don't need to merge it into the result region. The restriction is purely to
tell the optimizer what it can/cannot do rather.
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

…owed in LoweredSIL, update some tests by splitting them into their own Lowered file.
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

Going to do a full test just in case another test needs updating (trying to be careful).

@gottesmm
Copy link
Contributor Author

Slava wants to do post-commit review.

@gottesmm gottesmm merged commit 8178eb0 into swiftlang:main Dec 19, 2023
@gottesmm gottesmm deleted the pr-763529d8ee9f4164b82d03087fec25439cc1d315 branch December 19, 2023 18:22
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.

1 participant