[region-isolation] Track elements -> regions and regions -> consuming separately. #69787

gottesmm · 2023-11-10T21:03:16Z

This PR contains a few different improvements to the pass. The most important one is that we as a result of this change actually perform two separate data flows at the same time: one mapping elements to regions and a second that maps regions to transferring instructions. In contrast previously we would track transferring just by marking the region of an element as -1, losing the region information and not storing what instruction caused the transfer to occur. The reason why I am making this change is that:

To handle async let correctly, we need to be able to "untransfer" a region... to do that I need to know what elements belonged to a region even if the region was transferred.
Right now there is a bunch of logic around back tracking to find the transfer instruction that hits a require. There isn't any guarantee that we can find it and I have discovered in certain cases we are just squelching the error and not emitting anything. By tracking the instruction that caused the transfer to occur, we can nip that in the bud and always have the transfer instruction. As a benefit, we can delete a bunch of code. I haven't implemented this part yet.

The patches are:

In this patch I just added a new API to ApplySite called getOperandsWithoutSelf(). Just wasn't implemented before and I needed it.
This just involved adding some MARK: comments to organize TransferNonSendable.cpp better.
I noticed that we were only calling diagnoseFailures in one place and adding this helper just obscured what was happening at the callsite. So I inlined the function.
Once I eliminated diagnoseFailures, I realized that there also was a helper for iterating over the PartitionOps in a BlockState. There is no need to provide an indirect call (or cause inlining burdens for the compiler) to just iterate over an array. This makes the code simpler and easier to read and reduces the amount of code needed to be understood by the reader.
This is the main part of the patch where I begin to track element -> region separately from region -> transferring inst. It doesn't actually use that information yet since I am using the original implementation to test for correctness. In a subsequent commit I am going to write up this information and fix the diagnostics.
While doing that aforementioned promised patch that fixes the diagnostics, I wanted to make PartitionOps a noncopyable type since it contains potentially heap allocated memory. When I did that I found a few places where we were making copies without realizing it. Since it is just goodness to do this and I had it done, I decided to fold these changes into this PR.
We for a long time have been emitting these (N accesses found) diagnostics. These are meant for implementors and are not intended to be seen by users. Now that we are making further progress towards users actually using these diagnostics, it made sense to just eliminate this part of the diagnostic.
I noticed while working on the aforementioned promised patch that we were calling getExprForPartitionOp and just grabbing an expr from the SILLocation but storing it separately. Rather than do this, just return the SILLocation for the SILInstruction and use that as appropriate. This as an added benefit makes it so that we can emit diagnostics for cases where our parameter is using a statement (for instance in a for loop). As another benefit, I also changed how we stopped multiple errors from being emitted for the same source code to use the SILInstruction of the transfer site instead of the Expo of the transfer site (which we used to get via getExprForPartitionOp). So there is no effect.

Needed this API. Unwrapping the onion to make further commits easier to review.

… read. NFC.

…gnoseFailures. This is only used in one place in partition analysis which is a data structure that does computation. In contrast, we want BlockPartitionState to be more of a POD type of data that each BasicBlock has mapped to it. Simplifying the code. This also let me get rid of the translator field in BasicBlockState. We only need to pass it in as an argument to the constructor to initialize our translation. It doesn't need to be stored anymore.

…on ops rather than use a callback. There isn't a strong reason to use a callback here since we aren't ever composing transformations on partition ops. Better instead to go for simplicity and just iterate directly. If we start doing complex transformations over partition ops with composable APIs, we can always add this back in later. As a nice benefit, one doesn't need to worry that the callback API is hiding actual complexity... since just by using a for loop we communicate that nothing interesting is happening here. Just reducing the amount of code surface area in the pass.

…ion. What this does is really split the one dataflow we are performing into two dataflows we perform at the same time. The first dataflow is the region dataflow that we already have with transferring never occurring. The second dataflow is a simple gen/kill dataflow where we gen on a transfer instruction and kill on AssignFresh. What it tracks are regions where a specific element is transferred and propagates the region until the element is given a new value. This of course means that once the dataflow has converged, we have to emit an error not if the value was transferred, but if any value in its region was transferred.

Was experimenting with making PartitionOps a noncopyable type and I discovered these places where we copy PartitionOps when we could use a const reference. It is good not to copy PartitionOps since they potentially contain a heap allocated array. Sadly, my change to make PartitionOps noncopyable will have to wait until a forthcoming commit here I overhaul how we emit errors since that older code copies PartitionOps a lot and I would rather just delete that code and then fix PartitionOps. But these are on the surface safe changes that makes sense to get in separately to make that next patch easier to review.

…nostic. These are not actionable to the user.

…SILLocation. getExprForPartitionOp(...) just returned the expression from the loc of op.currentInst: SILInstruction *sourceInstr = op.getSourceInst(/*assertNonNull=*/true); Expr *expr = sourceInstr->getLoc().getAsASTNode<Expr>(); Instead of mucking around with exprs, just use the SILLocation from the SILInstruction. I also changed how we unique transfer instructions to just use the transfer instruction itself instead of the AST/Expr of the transfer instruction.

gottesmm · 2023-11-10T21:03:36Z

@swift-ci smoke test

slavapestov · 2023-11-13T18:27:29Z

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

-          });
+          };
+      eval.nonTransferrableElements = translator.getNeverTransferredValues();
+      eval.isActorDerivedCallback = [&](Element element) -> bool {


I wonder if these callbacks are worth it at all, do they allow unit testing or something?

I don't think they are. The idea is to make it so that one can visit/process the blocks and then reuse the same visiting code to visit/emit the errors. I'll see if I can come up with something better.

slavapestov · 2023-11-13T18:27:50Z

include/swift/SIL/ApplySite.h

@@ -485,6 +485,21 @@ class ApplySite {
    llvm_unreachable("covered switch");
  }

+  /// Return a list of applied operands of the apply without self.
+  ArrayRef<Operand> getOperandsWithoutSelf() const {


Are there any other places this can be used?

gottesmm added 8 commits November 10, 2023 12:48

[sil] Add new API to ApplySite called getOperandsWithoutSelf().

9764359

Needed this API. Unwrapping the onion to make further commits easier to review.

[region-isolation] Add some MARK: to make the file a little easier to…

80e1ebe

… read. NFC.

[region-isolation] Remove count of emitted diagnostics from main diag…

389078d

…nostic. These are not actionable to the user.

gottesmm requested review from ktoso, kavon, hborla, slavapestov and xedin as code owners November 10, 2023 21:03

slavapestov reviewed Nov 13, 2023

View reviewed changes

slavapestov approved these changes Nov 13, 2023

View reviewed changes

gottesmm merged commit 44e1e54 into swiftlang:main Nov 13, 2023

gottesmm deleted the region-isolation-track-consumption-separately branch November 13, 2023 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[region-isolation] Track elements -> regions and regions -> consuming separately. #69787

[region-isolation] Track elements -> regions and regions -> consuming separately. #69787

gottesmm commented Nov 10, 2023

Uh oh!

gottesmm commented Nov 10, 2023

Uh oh!

slavapestov Nov 13, 2023

Uh oh!

gottesmm Nov 13, 2023

Uh oh!

slavapestov Nov 13, 2023

Uh oh!

Uh oh!

[region-isolation] Track elements -> regions and regions -> consuming separately. #69787

[region-isolation] Track elements -> regions and regions -> consuming separately. #69787

Conversation

gottesmm commented Nov 10, 2023

Uh oh!

gottesmm commented Nov 10, 2023

Uh oh!

slavapestov Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

gottesmm Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

slavapestov Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!