[region-isolation] Fix actor region propagation and some other smaller issues. #69387

gottesmm · 2023-10-24T23:47:40Z

This PR contains a few different changes:

It teaches the pass how to handle actor regions. Put simply, we in a flow insensitive way identify if specific values are isolated to an actor and then if a region contains any value with an actor isolated value, then the whole region must be actor isolated. Thus we can take advantage of the region dataflow to make our flow insensitive constraint flow sensitive.
A hack was added some time ago that made it so that if an address was passed to any address, then we needed to make it so that assignments were actually merges (meaning that later assignments would not erase the old region a value belonged to). This should only semantically happen with partial_apply. As far as the codegen of the compiler is concerned, I was unable to come up with a case where the unnecessary merges were harmful. I could construct something with SIL but unfortunately we can't write SIL test cases yet with this pass.... = /.
I noticed that the whole cache vector of argument IDs for function arguments was only used to construct a Partition for the entry block. Rather than having this extra concept, I changed the function arguments to just initialize directly the partition... eliminating code.
I noticed that we were not properly handling begin_access. Specifically, begin_access was being treated as an assignment. That means that if one were to transfer a var and then attempt to reinitialize the var, the begin_access of the reinitialization would be considered a use of the transferred value and we would emit an error... which is incorrect. Instead, we just look through begin_access.
The pass as originally written is a pass that does a bunch of work deep in the callstack that doesn't need to be done so deep. By making it so deep, it obscures what the pass is actually trying to do. An example of this is that before this PR, we initialized a block's contained pseudo-IR lazily when we were performing dataflow. This is not necessary and makes it non-obvious where the block's translation is happening. Instead we just perform it for all blocks when we construct their state and before we perform the dataflow. As a bonus, this makes the logging easier to understand since we first translate all blocks to the pseudo-ir and then perform the dataflow instead of interleaving the two.

That being said, I found a few bugs in the process that I filed radars for that I will fix in subsequent PRs. I added tests for all of them into this PR with TODOs.

I discovered that in certain cases, I can get SILGen to initialize aggregate values separately in pieces. If a later value is a Sendable type that is fresh, we lose the original region of the value. This can cause us to swallow diagnostics. The solution is to make it so that if we are assigning into only one part of an aggregate, we merge into the region so we don't forget the original region. rdar://117273952
I discovered that when one returns a non-Sendable value, if we were going to emit an error on the return (since the return would be a use after a transfer lets say), then we do not have an expression associated with the value so we crash. This is why we really should be using SILLocations instead so we get proper diagnostics. rdar://117438350
I discovered that we are not properly performing a union operation when combining regions from different blocks. rdar://117437059.

…n actor and treat its region as being within an actor's isolation domain. Importantly, we determine at the error stage if a specific value that is transferred is within the same region of a value that is Actor derived. This means that we can in a flow insensitive manner determine the values that are actor derived and then via propagating those values into various regions determine the issue... using our region analysis to handle the flow sensitivity. One important case to reason about here is that since we are relying on the region propagation for flow sensitivity is that this solves the var case for us. A var when declared is never marked as actor derived. Var uses only become actor derived if the var was merged into a region that contain is actor derived. That means that re-assigning to a non-actor derived value, eliminates the actor derived bit. As part of this, I also discovered I could just get rid of the captured uniquely identified array in favor of just passing in an actor derived flag. rdar://115656589

…ress in a partial_apply rather than if we send it to any function apply. Semantically these are the only cases where we would want to force an address to be merged. Some notes: 1. When we have a box, the project_box is considered to come from the box and is considered non uniquely identified. 2. The only place this was really triggering was when we created temporary partial applies to pass to a function when using address only types. Since this temporaries are never reassigned to, there isn't any reason to require them to be merged. 3. In the case where we have an alloc_stack grabbed by a partial_apply, we still appropriately merge.

…s, just cache the function entry partition. We only ever used this cache of IDs to construct the function arg partition. Since a Partition involves using memory, it makes sense to just cache the Partition itself and eliminate the unnecessary second cache.

…reat it like an assignment. If we treat a begin_access as an assignment then in cases like below we assume that a value was used before we reassign. ``` func testVarReassignStopActorDerived() async { var closure = {} await transferToMain(closure) // This re-assignment shouldn't error. closure = {} // (1) } ``` rdar://117437299

…t all blocks during the dataflow... just do the translation when we initialize block state. We are going to visit all of the blocks anyways... so it makes sense to just run them as part of the initializing of the block state. Otherwise, the logic is buried in the dataflow which makes it harder for someone who is not familiar with the pass to reason about it.

…that they are derived from. Otherwise, it is hard to tell what one is looking at when looking at the pseudo-ir.

gottesmm · 2023-10-24T23:47:54Z

@swift-ci smoke test

slavapestov · 2023-10-25T18:28:37Z

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

+      return translateSILAssign(inst->getResult(0), inst->getOperand(0));
+    }
+
+    case SILInstructionKind::EnumInst: {


Is this case new? Does it have a test?

Yes. It came up with the enum test cases I added below. I wish we had SIL test cases which would make it easier to understand the connection.

slavapestov · 2023-10-25T18:29:30Z

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

@@ -339,7 +339,7 @@ class PartitionOpTranslator {
  std::vector<TrackableValueID> neverTransferredValueIDs;

  /// A cache of argument IDs.
-  SmallVector<TrackableValueID> argIDs;
+  std::optional<Partition> functionArgPartition;


Maybe make it mutable?

gottesmm added 6 commits October 24, 2023 16:35

[region-isolation] When printing PartitionOps, print the instruction …

30933db

…that they are derived from. Otherwise, it is hard to tell what one is looking at when looking at the pseudo-ir.

gottesmm requested a review from slavapestov October 24, 2023 23:47

gottesmm requested review from ktoso and kavon as code owners October 24, 2023 23:47

slavapestov approved these changes Oct 25, 2023

View reviewed changes

gottesmm merged commit c10c4fc into swiftlang:main Oct 25, 2023

gottesmm deleted the more-region-stuff branch October 25, 2023 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[region-isolation] Fix actor region propagation and some other smaller issues. #69387

[region-isolation] Fix actor region propagation and some other smaller issues. #69387

Uh oh!

gottesmm commented Oct 24, 2023

Uh oh!

gottesmm commented Oct 24, 2023

Uh oh!

slavapestov Oct 25, 2023

Uh oh!

gottesmm Oct 25, 2023

Uh oh!

slavapestov Oct 25, 2023

Uh oh!

Uh oh!

[region-isolation] Fix actor region propagation and some other smaller issues. #69387

[region-isolation] Fix actor region propagation and some other smaller issues. #69387

Uh oh!

Conversation

gottesmm commented Oct 24, 2023

Uh oh!

gottesmm commented Oct 24, 2023

Uh oh!

slavapestov Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!