[region-isolation] Do not look through begin_borrow or move_value if they are marked as a var_decl. #72959

gottesmm · 2024-04-10T20:11:52Z

Just chopping off parts of a larger piece of work while I work at the same time on another piece of work.

The reason why we do this is that we want to treat this as a separate value from
their operand since they are the result of defining a new value.

This has a few nice side-effects, one of which is that if a let results in just
a begin_borrow [var_decl], we emit names for it.

I also did a little work around helping variable name utils to lookup names from
applies that are fed into a {move_value,begin_borrow} [var_decl] which then has
the debug_value we are searching for.

gottesmm · 2024-04-10T20:11:57Z

@swift-ci smoke test

gottesmm · 2024-04-10T21:04:59Z

This is actually going to cause a merge conflict with a different PR I have going. I am going to do some more work here and rebase on top of that. That means I am going to put my SILIsolationInfo refactoring work here as well.

gottesmm · 2024-04-11T01:24:11Z

@swift-ci smoke test

gottesmm · 2024-04-11T02:06:33Z

@swift-ci smoke test

gottesmm · 2024-04-11T03:24:51Z

@swift-ci smoke test

ktoso · 2024-04-11T08:23:05Z

include/swift/SILOptimizer/Utils/PartitionUtils.h

+        isolatedValue(isolatedValue), actorInstance(actorInstance) {
+    assert((!actorInstance ||
+            (actorIsolation.getKind() == ActorIsolation::ActorInstance &&
+             actorInstance->getType().isActor())) &&


Suggested change

actorInstance->getType().isActor())) &&

actorInstance->getType().isAnyActor())) &&

otherwise would crash on a distributed actor

This actually calls:

bool TypeBase::isActorType() { if (auto actor = getAnyActor()) return actor->isActor(); return false; }

which does any actor.

Right, which is still wrong isn't it? It does actor->isActor() there but should do actor->isAnyActor().
The isActor checks for inheriting from Actor but we need either Actor or DistributedActor or this will fail when isolated to a distributed actor right?

ktoso · 2024-04-11T08:28:55Z

Not much I can say about the analysis bits but looked good :)

… isolation info. I am finding that I am calling that a bunch so this just makes it a little more convenient.

…ment. Having two artificial typedefs for the same wrapped value is just confusing. Better to just have one and make the code simpler to understand.

…they are marked as a var_decl. The reason why we do this is that we want to treat this as a separate value from their operand since they are the result of defining a new value. This has a few nice side-effects, one of which is that if a let results in just a begin_borrow [var_decl], we emit names for it. I also did a little work around helping variable name utils to lookup names from applies that are fed into a {move_value,begin_borrow} [var_decl] which then has the debug_value we are searching for.

…ment_addr and global_addr onto SILIsolationInfo and call that instead.

…efInst/ClassMethodInst into SILIsolationInfo::get instead of handrolling in RegionAnalysis.

…ationInfo::get instead of computing actor isolation by hand. Just more recoring on top of SILIsolationInfo::get.

…ionInfo::get.

This ensures that when we process, we consider load/load_borrow's result to be equivalent to its operand. This ensures that a load/load_borrow cannot act as a use of its operand preventing spurious diagnostics.

…ce that a value is isolated to if we are dealing with an actor instance. This will let us distinguish in between values derived from two actor instances of the same type and to emit better errors.

…o VariableNameInferrer. I have been using these in TransferNonSendable and they are useful in terms of reducing the amount of code that one has to type to use this API. I am going to need to use it in SILIsolationInfo, so it makes sense to move it into SILOptimizer/Utils. NFCI.

…iagnostics, if we have a SIL actor instance, print -isolated instead of actor-isolated. rdar://122501400

…ctor self. rdar://122501400

…ated to a #isolated as being globally isolated to that. Instead, we need to consider the isolation at the apply site. rdar://126285681 rdar://125078448

The thinko was that rather than use the actual isolated parameter of a partial apply as the isolated parameter, I instead just used the last operand... :doh:. This fixes: rdar://126297097

gottesmm · 2024-04-11T23:02:40Z

@swift-ci smoke test

gottesmm · 2024-04-11T23:02:49Z

Lets try this again

gottesmm · 2024-04-12T01:45:08Z

@swift-ci smoke test

gottesmm · 2024-04-12T02:07:54Z

@swift-ci smoke test

ktoso

Sorry but I still think this assertion will wrongly crash for distributed actor isolated code -- commented inline

… actors.

…isAnyActor().

I went through and converted all usages in TransferNonSendable to use isAnyActor instead of isActor as requested.

gottesmm · 2024-04-17T20:09:12Z

@swift-ci smoke test

gottesmm · 2024-04-17T20:09:50Z

I also included some tests around distributed actors as well to validate that we do not crash.

gottesmm requested a review from ktoso as a code owner April 10, 2024 20:11

gottesmm enabled auto-merge April 10, 2024 20:11

gottesmm disabled auto-merge April 10, 2024 21:04

gottesmm force-pushed the tns-upstream-2 branch from b74015c to 0a38a5b Compare April 11, 2024 01:24

gottesmm force-pushed the tns-upstream-2 branch from 0a38a5b to 75e7a40 Compare April 11, 2024 02:06

gottesmm force-pushed the tns-upstream-2 branch from 75e7a40 to dc13982 Compare April 11, 2024 03:24

ktoso reviewed Apr 11, 2024

View reviewed changes

ktoso approved these changes Apr 11, 2024

View reviewed changes

gottesmm added 15 commits April 11, 2024 15:41

[region-isolation] Add a small helper to TrackableValue to print it's…

beaab91

… isolation info. I am finding that I am calling that a bunch so this just makes it a little more convenient.

[region-isolation] Eliminate unnecessary using TrackableValueID = Ele…

c9fe8ff

…ment. Having two artificial typedefs for the same wrapped value is just confusing. Better to just have one and make the code simpler to understand.

[region-isolation] Rename Partition::{fresh_label,freshLabel}.

b80faee

[region-isolation] Move SILIsolationInfo determining code for ref_ele…

513ab78

…ment_addr and global_addr onto SILIsolationInfo and call that instead.

[region-isolation] Move computation of SILIsolationInfo for FunctionR…

2a7714a

…efInst/ClassMethodInst into SILIsolationInfo::get instead of handrolling in RegionAnalysis.

[region-isolation] Change load [copy]/load_borrow to just use SILIsol…

baca235

…ationInfo::get instead of computing actor isolation by hand. Just more recoring on top of SILIsolationInfo::get.

[region-isolation] Finish moving isolation computation into SILIsolat…

b407b21

…ionInfo::get.

[region-isolation] Make load/load_borrow look through instructions.

eed51e7

This ensures that when we process, we consider load/load_borrow's result to be equivalent to its operand. This ensures that a load/load_borrow cannot act as a use of its operand preventing spurious diagnostics.

[region-isolation] Begin tracking in SILIsolationInfo the actorInstan…

d8f39f7

…ce that a value is isolated to if we are dealing with an actor instance. This will let us distinguish in between values derived from two actor instances of the same type and to emit better errors.

[region-isolation] When printing a SILIsolationInfo description for d…

62a4820

…iagnostics, if we have a SIL actor instance, print -isolated instead of actor-isolated. rdar://122501400

[region-isolation] Emit the correct error for closures that capture a…

a9c163f

…ctor self. rdar://122501400

[region-isolation] Do not treat functions/class_methods that are isol…

2e5b3bc

…ated to a #isolated as being globally isolated to that. Instead, we need to consider the isolation at the apply site. rdar://126285681 rdar://125078448

[region-isolation] Fix thinko.

969dd69

The thinko was that rather than use the actual isolated parameter of a partial apply as the isolated parameter, I instead just used the last operand... :doh:. This fixes: rdar://126297097

gottesmm force-pushed the tns-upstream-2 branch from dc13982 to 969dd69 Compare April 11, 2024 23:02

gottesmm enabled auto-merge April 11, 2024 23:02

Remove pragma clang optimize off that snuck in.

9d965f8

ktoso previously requested changes Apr 12, 2024

View reviewed changes

gottesmm added 2 commits April 17, 2024 13:07

[region-isolation] Add some tests around initializers and distributed…

fa20e52

… actors.

[region-isolation] Out of an abundance of caution convert isActor -> …

3c29997

…isAnyActor().

gottesmm merged commit bfa910d into swiftlang:main Apr 18, 2024

gottesmm deleted the tns-upstream-2 branch April 18, 2024 05:39

gottesmm mentioned this pull request Apr 19, 2024

[6.0][region-isolation] Cherry-pick recent work #73150

Merged

	actorInstance->getType().isActor())) &&
	actorInstance->getType().isAnyActor())) &&

[region-isolation] Do not look through begin_borrow or move_value if they are marked as a var_decl. #72959

[region-isolation] Do not look through begin_borrow or move_value if they are marked as a var_decl. #72959

Uh oh!

Conversation

gottesmm commented Apr 10, 2024

Uh oh!

gottesmm commented Apr 10, 2024

Uh oh!

gottesmm commented Apr 10, 2024

Uh oh!

gottesmm commented Apr 11, 2024

Uh oh!

gottesmm commented Apr 11, 2024

Uh oh!

gottesmm commented Apr 11, 2024

Uh oh!

ktoso Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

gottesmm Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

ktoso Apr 12, 2024 • 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!

ktoso commented Apr 11, 2024

Uh oh!

gottesmm commented Apr 11, 2024

Uh oh!

gottesmm commented Apr 11, 2024

Uh oh!

gottesmm commented Apr 12, 2024

Uh oh!

gottesmm commented Apr 12, 2024

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Apr 17, 2024

Uh oh!

gottesmm commented Apr 17, 2024

Uh oh!

Uh oh!

ktoso Apr 12, 2024 •

edited

Loading