[region-based-isolation] Stack of smaller fixes before a larger fix #69342

gottesmm · 2023-10-23T21:00:27Z

This PR contains a small set of fixes that are chopped off a larger PR. Most are NFC or small performance fixes. I am chopping it off to make reviewing the other PR easier.

The specific PRs are:

I added a new helper API to SILType to check if a SILType is an actor. This prevents us from needing to get at the ASTType to get that information.
I added new verbose logging to PartitionUtils.h so that as the partition utils walks the blocks, one can see how it changes the regions. Otherwise the only information one gets from the logging are the exiting regions at the bottom of the block which is not always the information one needs. Since this is significantly more verbose, I put it behind a flag.
Since I added the flag from 2, I needed to add a new .cpp file for PartitionUtils. Since PartitionUtils is in SILOptimizer, I had to also create that .cpp file in SILOptimizer. This required me to move the unit tests for PartitionUtils from the SIL unittest folder to the SILOptimizer unittest folder so that we link against the appropriate library.
I found another case of a std::vector being used as an argument where we could just use an ArrayRef since we are never writing into the vector. Eliminating unnecessary heap allocations are good.
I deleted an incorrect assert. The assert was validating that if we transferred a value within the block, we always had the value as transferred at the end of the block. This isn't true if we reinitialize the value within the block.
I removed an unnecessary helper that checked if a value has an address type.
I changed the ConsumeRequireAccumulator to just emit errors directly rather than use callbacks. I did this since it seemed like a premature optimization.
I finally changed the rest of the usages of Consume -> Transferred since that is the terminology we are using.

Just a helper that calls getASTType()->isActorType(). I also updated the two places in SNS that we use this.

This is the correct thing to do since the header is in SILOptimizer. That being said the reason why I am doing this is that I want to add a command line flag to PartitionUtils.h to allow for more verbose debug output and the flag's definition will be in the SILOptimizer library. So this is just a little cleanup that follows from that.

One needs to pass in the explicit flag to enable this as well as -debug-flag=send-non-sendable. This makes it easier to debug the affect of applying specific partition ops.

We can just pass in an ArrayRef here. I missed this inefficiency when I was going through the pass earlier.

If we consume a value in a block and then reinitialize it before the end of the block, this assert will always fail.

SILValues always have a SILType... so just use getType().isAddress().

…rather than passing in callbacks. This indirection is premature abstraction since the only two places we actually use these callbacks is to print the accumulated output and to emit our errors. With this change, I was able to simplify how print works and inline in the actual diagnostic emission making it easier to understand the code. Additionally, in a subsequent commit I am going to need to ensure that we do not emit an error here if the source cause of our error is due to an actor. To implement that I would need to change tryDiagnoseAsCallsite to signal to the machinery that it needs to not emit requires and then fail further up as well. Given that there really wasn't a need for this abstraction in the first place and the amount of munging needed to express this, I decided to just make this change and make it easier. If we truly need this flexibility, we can always add it back later.

…me -> Transfer. I just missed these.

gottesmm · 2023-10-23T21:01:12Z

@swift-ci smoke test

xedin · 2023-10-24T16:30:23Z

lib/SILOptimizer/Mandatory/SendNonSendable.cpp


  bool containsOp(const PartitionOp& op) {
-    for (auto [_, vec] : consumeOps)
+    for (auto [_, vec] : transferOps)


I think this and isValid() could be further simplified to use llvm::any_of and llvm::is_contained as a follow-up.

@xedin TBH, I really dislike containsOp. It would be much better to just have a separate set like thing. So we can do quick PartitionOp checks. But I am going to leave it in for now since this only runs when we emit error diagnostics and fail.

xedin · 2023-10-24T16:30:45Z

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

-class ConsumedReason {
-  std::map<unsigned, std::vector<PartitionOp>> consumeOps;
+class TransferredReason {
+  std::map<unsigned, std::vector<PartitionOp>> transferOps;


A good use for std::multimap

xedin · 2023-10-24T16:34:57Z

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

+    auto isolationCrossing = apply->getIsolationCrossing();
+    if (!isolationCrossing) {
+      assert(false && "ApplyExprs should be transferring only if"
+                      " they are isolation crossing");


Why not just

assert(isolationCrossing && "ApplyExprs should be transferring only if" " they are isolation crossing");

xedin · 2023-10-24T16:35:12Z

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

+    auto argExpr = transferOp.getSourceExpr();
+    if (!argExpr)
+      assert(false &&
+             "sourceExpr should be populated for ApplyExpr consumptions");


I think this should be assert(argExpr && ...) instead.

gottesmm · 2023-10-24T17:18:42Z

@xedin I am going to make these changes in a follow on commit. Thanks!

slavapestov · 2023-10-24T17:42:01Z

include/swift/SIL/SILType.h

@@ -878,6 +878,8 @@ class SILType {

  bool isMarkedAsImmortal() const;

+  bool isActor() const { return getASTType()->isActorType(); }


Why not name it isActorType() for consistency with the CanType method?

My thought was that we already know it is a type, so no point in adding that redundant text.

slavapestov · 2023-10-24T17:43:16Z

include/swift/SILOptimizer/Utils/PartitionUtils.h

+  // transferred. Additionally, a list of "nonconsumable" indices can be passed
+  // in along with a handleConsumeNonConsumable closure. In the event that a
+  // region containing one of the nonconsumable indices is transferred, the
+  // closure will be called with the offending Consume.


with the offending Transfer?

slavapestov · 2023-10-24T17:43:41Z

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

@@ -246,7 +246,7 @@ struct PartitionOpBuilder {

  void addTransfer(SILValue value, Expr *sourceExpr = nullptr) {
    assert(valueHasID(value) &&
-           "consumed value should already have been encountered");
+           "transfered value should already have been encountered");


Typo: transferred

…d87d1532aff686 [region-isolation] Follow up changes to #69342

gottesmm added 8 commits October 23, 2023 12:24

[sil] Implement SILType::isActor().

d598da5

Just a helper that calls getASTType()->isActorType(). I also updated the two places in SNS that we use this.

[region-isolation] Add verbose logging to PartitionUtils behind a flag.

fb7b4a3

One needs to pass in the explicit flag to enable this as well as -debug-flag=send-non-sendable. This makes it easier to debug the affect of applying specific partition ops.

[region-isolation] Eliminate a temporary std::vector.

578eafd

We can just pass in an ArrayRef here. I missed this inefficiency when I was going through the pass earlier.

Delete an incorrect assert

a8886e7

If we consume a value in a block and then reinitialize it before the end of the block, this assert will always fail.

[region-isolation] Eliminate unnecessary helper.

cbc1241

SILValues always have a SILType... so just use getType().isAddress().

[region-isolation] Go through the implementation again changing Consu…

2c0efe6

…me -> Transfer. I just missed these.

gottesmm requested review from hborla, slavapestov and xedin as code owners October 23, 2023 21:00

gottesmm changed the title ~~[region-based-isolation]~~ [region-based-isolation] Stack of smaller fixes before a larger fix Oct 23, 2023

xedin approved these changes Oct 24, 2023

View reviewed changes

gottesmm merged commit bf64711 into swiftlang:main Oct 24, 2023

slavapestov reviewed Oct 24, 2023

View reviewed changes

gottesmm deleted the small-region-isolation-fixes branch October 24, 2023 17:55

gottesmm mentioned this pull request Oct 24, 2023

[region-isolation] Follow up changes to #69342 #69378

Merged

gottesmm added a commit that referenced this pull request Oct 25, 2023

Merge pull request #69378 from gottesmm/pr-cef756d1ce1215ea9d5eeb3500…

b67fc90

…d87d1532aff686 [region-isolation] Follow up changes to #69342

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[region-based-isolation] Stack of smaller fixes before a larger fix #69342

[region-based-isolation] Stack of smaller fixes before a larger fix #69342

Uh oh!

gottesmm commented Oct 23, 2023

Uh oh!

gottesmm commented Oct 23, 2023

Uh oh!

xedin Oct 24, 2023

Uh oh!

gottesmm Oct 24, 2023 •

edited

Loading

Uh oh!

xedin Oct 24, 2023

Uh oh!

xedin Oct 24, 2023

Uh oh!

xedin Oct 24, 2023

Uh oh!

gottesmm commented Oct 24, 2023

Uh oh!

slavapestov Oct 24, 2023

Uh oh!

gottesmm Oct 24, 2023

Uh oh!

slavapestov Oct 24, 2023

Uh oh!

slavapestov Oct 24, 2023

Uh oh!

Uh oh!

		@@ -878,6 +878,8 @@ class SILType {

		bool isMarkedAsImmortal() const;

		bool isActor() const { return getASTType()->isActorType(); }

[region-based-isolation] Stack of smaller fixes before a larger fix #69342

[region-based-isolation] Stack of smaller fixes before a larger fix #69342

Uh oh!

Conversation

gottesmm commented Oct 23, 2023

Uh oh!

gottesmm commented Oct 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Oct 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gottesmm Oct 24, 2023 •

edited

Loading