Skip to content

[pmo] When exploding a copy_addr that assigns into memory, treat the … #22012

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.

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

Conversation

gottesmm
Copy link
Contributor

…resulting store as an init, not an assign.

This models the semantics we are trying to have here more closely without
changing current output. Specifically, an assign in this case is supposed to
mean that the underlying value is overwritten and destroyed (via a ref count
decrement). This is obviously true with the non-init copy_addr form, i.e.:

%0 = alloc_stack $Builtin.NativeObject
%1 = alloc_stack $Builtin.NativeObject
...
copy_addr [take] %0 to %1 : $*Builtin.NativeObject
...

Notice how the instruction is actively going to destroy whatever is in %1. Lets
consider the same SIL after exploding the copy_addr.

%0 = alloc_stack $Builtin.NativeObject
%1 = alloc_stack $Builtin.NativeObject
...
%2 = load %0 : $*Builtin.NativeObject
%3 = load %1 : $*Builtin.NativeObject (1)
store %2 to %1 : $*Builtin.NativeObject
destroy_value %3 : $Builtin.NativeObject (2)
...

In this case, the store is actually acting like an initialization since the
destructive part of the assign is performed by (1), (2). So considering the
store to be an assign is incorrect since a store that is an assign /should/
destroy the underlying value itself.

In terms of the actual effect on the result of the pass today, Initialization
and Assign stores are treated the same when it comes to getting available values
from stores and (for stores) destroying allocations. So this should be an NFC
change, but gets us closer to the model where assigns are only used for things
that store into memory and destroy the underlying model directly.

…resulting store as an init, not an assign.

This models the semantics we are trying to have here more closely without
changing current output. Specifically, an assign in this case is supposed to
mean that the underlying value is overwritten and destroyed (via a ref count
decrement). This is obviously true with the non-init copy_addr form, i.e.:

  %0 = alloc_stack $Builtin.NativeObject
  %1 = alloc_stack $Builtin.NativeObject
  ...
  copy_addr [take] %0 to %1 : $*Builtin.NativeObject
  ...

Notice how the instruction is actively going to destroy whatever is in %1. Lets
consider the same SIL after exploding the copy_addr.

  %0 = alloc_stack $Builtin.NativeObject
  %1 = alloc_stack $Builtin.NativeObject
  ...
  %2 = load %0 : $*Builtin.NativeObject
  %3 = load %1 : $*Builtin.NativeObject      (1)
  store %2 to %1 : $*Builtin.NativeObject
  destroy_value %3 : $Builtin.NativeObject   (2)
  ...

In this case, the store is actually acting like an initialization since the
destructive part of the assign is performed by (1), (2). So considering the
store to be an assign is incorrect since a store that is an assign /should/
destroy the underlying value itself.

In terms of the actual effect on the result of the pass today, Initialization
and Assign stores are treated the same when it comes to getting available values
from stores and (for stores) destroying allocations. So this should be an NFC
change, but gets us closer to the model where assigns are only used for things
that store into memory and destroy the underlying model directly.
@gottesmm gottesmm requested review from atrick and eeckstein January 20, 2019 19:41
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test and merge

@gottesmm gottesmm requested a review from shajrawi January 20, 2019 19:42
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit a57dee1 into swiftlang:master Jan 20, 2019
@gottesmm gottesmm deleted the pr-6bbbe995ebae0d51fccdfcb1cf06536fb8e0e5f3 branch January 20, 2019 20:59
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants