Skip to content

[region-isolation] Since we now propagate the transferred instruction, use that to emit the error instead of attempting to infer the transfer instruction for a requires #69906

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 16, 2023
Merged

[region-isolation] Since we now propagate the transferred instruction, use that to emit the error instead of attempting to infer the transfer instruction for a requires #69906

merged 8 commits into from
Nov 16, 2023

Conversation

@gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Nov 16, 2023

In this PR, I transition the pass from attempting to infer from regions transfer instructions, just use the newly propagated transfer instruction.

Here is a guide to the PR.

  1. ca663b1. I noticed while working on this that destructureXInst were not viewed as look through instructions when they should be. Their result from a region perspective doesn't form new memory from its operand it doesn't have side-effects... so it makes sense to make it look through. Just fighting another battle in my war to move all instructions that should be lookThrough to lookThrough.
  2. c929995. I added the ability to grab a type erased ASTNode out of a SILLocation. The problem this is fixing is that one could get an AST type out, one just needed to know what the class was. This made it so that one could not separate the grabbing of the ASTNode from the SILLocation and performing the unwrapping of the inner ASTNodeTy. I was originally going to use this in the implementation but did not need it. It seems like a missing API on SILLocation.
  3. 9e3f0b0. This commit just made a rename I have wanted to make for a while and this was an opportunity to sneak it in. Just renaming SILApplyCrossesIsolation -> isIsolationBoundaryCrossingApply.
  4. fc73210. I actually hit this independent issue when doing the larger work of this PR. The issue is that one could have a Sendable type that when run through the getUnderlyingObject returned a non-Sendable value. The pass logic would then process the Sendable value like a non-Sendable value. An example of where this happens is pointer_to_address from a Builtin.RawPointer (which is non-Sendable) to a sendable address. Once we know that the type is actually Sendable the correct semantics must be that we stop tracking it. If the user is doing this it is an unsafe transformation that the compiler should respect.

The next set of patches are tied together and have to do with the actual main work of this PR. I split it up to make it easier to review.

  1. 957a79f. In the previous patch of commits I made it so that we tracked the SILInstruction of the transfer and transitioned parts of the Transfer PartitionOp API to use Operands... but I didn't wire it up. In this commit, I wire that functionality up so that we now actually emit the transfer error using the Operand. The reason that I did this is that I needed the SILValue of the transfer operand that caused the violation in a later patch and noted that an Operand still lets me get the instruction that caused the issue. But, we still emit diagnostics after the dataflow has occurred using the old instruction model.
  2. 95669c5. Now that we propagate the operand of the transfer, I was able to do a nice refactor that changed how we got the expr of the initial type. Specifically, previously we used to really early figure out the expr for a specific PartitionOp and then propagate that around in the PartitionOp through the entire computation and then use it to lookup the type of the value if we emit the error. In this patch I move the computation of this expr to the place where we would actually emit the error and eliminated the expr field from PartitionOp.
  3. c0b3efe. I changed how we emitted diagnostics in the pass to make it more robust and easier to understand. Previously there was a complex set of code that was used so that we could emit many requires (limiting the requires we found to 50) and it generally went from requires -> transfers. Now that we just propagate transfers to requires all of this work isn't necessary. To ensure that we only emit a reasonable amount of requires, I changed the pass to just emit the first requires along a path.
  4. 46c4da3. This commit just removes the old implementation. I found that when I did it in the same commit as the previous commit, it was hard to read the output.

@gottesmm
[region-isolation] Make it possible to "lookThrough" multiple result …
ca663b1 
…instructions and use it to lookthrough destructures.

Currently when one says that an instruction is not a "look through" instruction,
each of its results gets a separate element number and we track these results as
independent entities that can be in a region. The one issue with this is
whenever we perform this sort of operation we actually are at the same time
performing a require on the operand of the instruction. This causes us to emit
errors on non-side effect having instructions when we really want to emit an
error on their side-effect having results. As an example of the world before
this patch, the following example would force the struct_element_addr to have a
require so we would emit an error on it instead of the apply (the thing that we
actually care about):

```
%0 = ...

// We transferred %0, so we cannot use it again.
apply %transfer(%0)

// We track %1 and %0 as separate elements and we treat this as an assignment of
// %0 into %1 which forces %0 to be required live at this point causing us to
// emit an error here...
%1 = struct_element_addr %0

// Instead of in the SIL here on the actual side-effect having instruction.
apply %actualUse(%1)
```

the solution is to make instructions like struct_element_addr lookthrough
instructions which force their result to just be the same element as their
operand. As part of doing this, we have to ensure that getUnderlyingTrackedValue
knows how to look through these types. This ensures that they are not considered
roots.

----

As an aside to implement this I needed to compose some functionality ontop of
getUnderlyingObject (specifically the look through behavior on destructures) in
a new helper routine called getUnderlyingTrackedObjectValue(). It just in a loop
calls getUnderlyingObject() and looks through destructures until its iterator
doesn't change.
@gottesmm
[sil] Add the ability to grab an ASTNode out of a SILLocation.
c929995 
Previously if one wanted to get an ASTNode, one needed to use the
getAsASTNode<T>() type. If one just wants to get out the type and use it in a
generic way using ASTNode there wasn't any way to do this... so I did it. We
actually have to do the marshalling here since ASTNodeTy and ASTNode have
different layouts despite them both being PointerUnions. So one can't just cast
in between them =---(.
@gottesmm
Rename SILApplyCrossesIsolation -> isIsolationBoundaryCrossingApply.
9e3f0b0 
Just a better name with better camelCasing.
@gottesmm
[region-isolation] Do not look through type casts from a non-Sendable…
fc73210 
… to a Sendable value.

This came up while I was debugging test cases from the other parts of this
work. The specific issue was around a pointer_to_address from a
RawPointer (which is considered non-Sendable) to a Sendable type. We were
identifying the RawPointer as being the representative of the Sendable value
implying we were processing Sendable values like they were
non-Sendable. =><=. I wish we had SIL test cases for region isolation since I
would add one for this...
@gottesmm
[region-isolation] Track operands instead of SILInstructions for Tran…
957a79f 
…sfer instructions.

This is another NFC refactor in preparation for changing how we emit
errors. Specifically, we need access to not only the instruction, but also the
specific operand that the transfer occurs at. This ensures that we can look up
the specific type information later when we emit an error rather than tracking
this information throughout the entire pass.
@gottesmm
[region-isolation] Instead of passing around an expression to get the…
95669c5 
… original type, just derive the type from the transfer operand when we emit the error.
@gottesmm
[region-isolation] Instead of using the complex logic to emit all pos…
c0b3efe 
…sible requires... just emit a diagnostic on the first require that we see.

This involved me removing the complex logic for emitting diagnostics we have
today in favor of a simple diagnostic that works by:

1. Instead of searching for transfer instructions, we use the transfer
instruction that we propagated by the dataflow... so there is no way for us to
possible not identify a transfer instruction... we always have it.

2. Instead of emitting diagnostics for all requires, just emit a warning for the
first require we see along a path. The reason that we need to do this is that in
certain cases we will have multiple "require" instructions from slightly
different source locations (e.x.: differing by column) but on the same line. I
saw this happen specifically with print where we treat stores into the array as
a require as well as the actual call to print itself where we pass the array.

An additional benefit of this is that this allowed me to get rid of the
cache of already seen require instructions. By doing this, we now emit errors
when the same apply needs to be required by different transfer instructions for
different arguments.

NOTE: I left in the original implementation code to make it easier to review
this new code. I deleted it in the next commit. Otherwise the git diff for this
patch is too difficult to read.
@gottesmm
Delete new dead code
46c4da3
@gottesmm gottesmm requested a review from slavapestov November 16, 2023 05:37
@gottesmm gottesmm requested review from kavon and ktoso as code owners November 16, 2023 05:37
@gottesmm
Copy link
Contributor Author

gottesmm commented Nov 16, 2023

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

gottesmm commented Nov 16, 2023

@swift-ci smoke test linux platform

@gottesmm
Copy link
Contributor Author

gottesmm commented Nov 16, 2023

I just talked with Slava... we are going to do post commit review on this.

@gottesmm gottesmm merged commit 7680332 into swiftlang:main Nov 16, 2023
@gottesmm gottesmm deleted the use-tracked-transfer-inst branch November 16, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slavapestov slavapestov Awaiting requested review from slavapestov

@ktoso ktoso Awaiting requested review from ktoso ktoso is a code owner

@kavon kavon Awaiting requested review from kavon

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@gottesmm