Skip to content
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

Add an effects(releasenone) function effects attribute #14924

Merged

Conversation

aschwaighofer
Copy link
Contributor

A @effects(releasenone) function might read/write global state but does not perform a release.

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test

@aschwaighofer
Copy link
Contributor Author

CC @lorentey @milseman

Let me know if that works for you.

lib/AST/Attr.cpp Outdated
case EffectsKind::ReadWrite:
case EffectsKind::ReleaseNone:
return "effects(releasenone)";
case EffectsKind::ReadWrite:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unintentional outdent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I have fixed this in a later commit.

@milseman
Copy link
Member

milseman commented Mar 2, 2018

Do you have a test case where code without the annotation is not ARC optimized, while code with it is?

@aschwaighofer
Copy link
Contributor Author

aschwaighofer commented Mar 2, 2018

public class Foobar {
  var x: Int = 1
}

var g: Foobar = Foobar()

@effects(releasenone)
@inline(never)
public func foobar(_ x: Int) -> Int {
  // This store makes the function not a valid @effects(releasenone) function.
  // Because the original object referenced by g is released. This store was only
  // added to demonstrate the effect of the release attribute on ARC in a single file.
  g = Foobar()
  return 2 * x
}

@inline(never)
public func froz(_ f : Foobar) {
  g = Foobar()
}

public func useFoobar(_ f: Foobar) -> Foobar {
  var x = f
  foobar(3)
  froz(x)
  return x
}

public func test() {
  var f = Foobar()
  f = useFoobar(f)
  print(f)
}

Compare the SIL of useFoobar with and without the @effects(releasenone) attribute.

// useFoobar(_:)
sil @$S11TestEffects9useFoobaryAA0D0CADF : $@convention(thin) (@owned Foobar) -> @owned Foobar {
// %0                                             // users: %8, %1
bb0(%0 : $Foobar):
  debug_value %0 : $Foobar, let, name "f", argno 1 // id: %1
  %2 = integer_literal $Builtin.Int64, 3          // user: %3
  %3 = struct $Int (%2 : $Builtin.Int64)          // user: %5
  // function_ref foobar(_:)
  %4 = function_ref @$S11TestEffects6foobaryS2iF : $@convention(thin) (Int) -> Int // user: %5
  %5 = apply %4(%3) : $@convention(thin) (Int) -> Int
  // function_ref specialized froz(_:)
  %6 = function_ref @$S11TestEffects4frozyyAA6FoobarCFTf4d_n : $@convention(thin) () -> () // user: %7
  %7 = apply %6() : $@convention(thin) () -> ()
  return %0 : $Foobar                             // id: %8
}
// useFoobar(_:)
sil [thunk] [always_inline] @$S11TestEffects9useFoobaryAA0D0CADF : $@convention(thin) (@owned Foobar) -> @owned Foobar {
// %0                                             // users: %9, %10, %5, %1
bb0(%0 : $Foobar):
  debug_value %0 : $Foobar, let, name "f", argno 1 // id: %1
  %2 = integer_literal $Builtin.Int64, 3          // user: %3
  %3 = struct $Int (%2 : $Builtin.Int64)          // user: %6
  // function_ref foobar(_:)
  %4 = function_ref @$S11TestEffects6foobaryS2iF : $@convention(thin) (Int) -> Int // user: %6
  strong_retain %0 : $Foobar                      // id: %5
  %6 = apply %4(%3) : $@convention(thin) (Int) -> Int
  // function_ref specialized froz(_:)
  %7 = function_ref @$S11TestEffects4frozyyAA6FoobarCFTf4d_n : $@convention(thin) () -> () // user: %8
  %8 = apply %7() : $@convention(thin) () -> ()
  strong_release %0 : $Foobar                     // id: %9
  return %0 : $Foobar                             // id: %10
} // end sil function '$S11TestEffects9useFoobaryAA0D0CADF'

@lorentey
Copy link
Member

lorentey commented Mar 2, 2018

Looking good 🎉 I've seen the exact same sort of retain/releases around opaque hasher calls; this looks to be exactly what we need.

(I'm working on reverting #14913 to use a simple inout hasher; once that's done, I'll run benchmarks with/without the new attribute to verify.)

@milseman
Copy link
Member

milseman commented Mar 2, 2018

@aschwaighofer is that test part of this PR? Where can I see the effects of this and how do I know what is and isn't guaranteed?

@aschwaighofer
Copy link
Contributor Author

No this test is not part of the commit. Right now you have to read the commit message.

This commit only tests that side effect analysis returns that a function may read or write but does not release. Everything else follows from that.

When I come back from vacation I will add something to the docs.

@aschwaighofer
Copy link
Contributor Author

The test case above is confusing (because it actually does release). I only did it this way to be able to test single file (without the store to the globale side effects analysis would have figured norelease on its own).

releasenone = may read, may write, but (as if) no release happens

@atrick
Copy link
Contributor

atrick commented Mar 2, 2018

Not to hold this up, since it's immediately required and doesn't make things too much worse, but...

Effects annotations will only be decipherable to me when we have a 1-1 mapping from attribute to effect. To get the equivalent of today's [readnone] you would need "[noread] [nowrite] [noretain] [norelease]", with only some combinations legal. i.e. you can't use any other effect attribute unless you also explicitly write [norelease].

Then we need the param-only variants, [noglobalread] [noglobalwrite] (which also require [norelease]). I don't think it makes sense to talk about global retain/release.

Then, in the optimization guide we can just define each of these attributes independently with the caveat that releasing may involve arbitrary reads or writes.

@atrick
Copy link
Contributor

atrick commented Mar 2, 2018

Oh, we may want to be able to use annotations to say we release without accessing globals or writing in a deinit, I just think it's error prone to add constraints by omission. i.e. "[noglobalread] [noglobalwrite]" would still be an error without also adding some sort of "deinit" attribute.

A ``@effects(releasenone)`` function might read/write global state but does not
perform a release.
@aschwaighofer aschwaighofer force-pushed the wip_effects_release_none branch from d98f66f to 6fe0633 Compare March 5, 2018 15:12
@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test

@aschwaighofer
Copy link
Contributor Author

aschwaighofer commented Mar 5, 2018

@milseman Second attempt at the example. I need two files to make it less confusing. Without -wmo the optimizer does not see the function bodies across file boundaries and cannot reason about whether a function may release or not. A function becomes a black box (except for its annotations).

$ cat TestEffects.swift
public class Foobar {
  var x: Int = 1
}

var g: Foobar = Foobar()

@inline(never)
public func mayRelease(_ f : Foobar) {
  g = f
}

public func useFoobar(_ f: Foobar) -> Foobar {
  var x = f
  noReleaseFunction(3)
  mayRelease(x)
  return x
}
$ cat TestEffects2.swift 
@effects(releasenone)
@inline(never)
public func noReleaseFunction(_ x: Int) -> Int {
  return 2 * x
}

With the annotation the ARC optimizer knows that noReleaseFunction(3) cannot release f and therefore the copy x =f is delayed until before the call to mayRelease(x).

$ swift-macosx-x86_64/bin/swift -frontend -emit-sil -primary-file TestEffects.swift TestEffects2.swift -O -module-name main -o - | less

sil @$S4main9useFoobaryAA0C0CADF : $@convention(thin) (@owned Foobar) -> @owned Foobar {
bb0(%0 : $Foobar):
  %2 = integer_literal $Builtin.Int64, 3 
  %3 = struct $Int (%2 : $Builtin.Int64)
  // function_ref noReleaseFunction(_:)
  %4 = function_ref @$S4main17noReleaseFunctionyS2iF : $@convention(thin) (Int) -> Int
  %5 = apply %4(%3) : $@convention(thin) (Int) -> Int
  // function_ref mayRelease(_:)
  %6 = function_ref @$S4main10mayReleaseyyAA6FoobarCF : $@convention(thin) (@owned Foobar) -> ()
==>  strong_retain %0 : $Foobar
  %8 = apply %6(%0) : $@convention(thin) (@owned Foobar) -> ()
  return %0 : $Foobar
}

Compare to:

$ cat TestEffects2.swift 
//@effects(releasenone)
@inline(never)
public func noReleaseFunction(_ x: Int) -> Int {
  return 2 * x
}
$ swift-macosx-x86_64/bin/swift -frontend -emit-sil -primary-file TestEffects.swift TestEffects2.swift -O -module-name main -o - | less

sil @$S4main9useFoobaryAA0C0CADF : $@convention(thin) (@owned Foobar) -> @owned Foobar {
bb0(%0 : $Foobar):
  %2 = integer_literal $Builtin.Int64, 3
  %3 = struct $Int (%2 : $Builtin.Int64)
  // function_ref noReleaseFunction(_:)
  %4 = function_ref @$S4main17noReleaseFunctionyS2iF : $@convention(thin) (Int) -> Int
==>  strong_retain %0 : $Foobar
  %6 = apply %4(%3) : $@convention(thin) (Int) -> Int
  // function_ref mayRelease(_:)
  %7 = function_ref @$S4main10mayReleaseyyAA6FoobarCF : $@convention(thin) (@owned Foobar) -> ()
  %8 = apply %7(%0) : $@convention(thin) (@owned Foobar) -> ()
  return %0 : $Foobar
}

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Mar 5, 2018

Build failed
Swift Test OS X Platform
Git Sha - d98f66f0b1315989abed90d898f83af02e5c9090

@swift-ci
Copy link
Contributor

swift-ci commented Mar 5, 2018

Build failed
Swift Test Linux Platform
Git Sha - d98f66f0b1315989abed90d898f83af02e5c9090

Copy link
Member

@milseman milseman left a comment

Choose a reason for hiding this comment

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

LGTM

strong_release %0 : $Builtin.NativeObject
%10 = tuple()
return %10 : $()
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@milseman
Copy link
Member

milseman commented Mar 5, 2018

@aschwaighofer (out of scope for this PR) how does the optimizer test optimizations across module boundaries? How does the optimizer test @_inlineable et al? Surely there's some kind of infrastructure to mock up and test SIL with different levels of visibility.

@aschwaighofer
Copy link
Contributor Author

Most of the SILOptimizer test cases are written in SIL. In SIL you can declare but not define a function. See for example the test case I added. This way the test case controls whether the implementation is available or not.

If you write a test case with swift source you can compile in -wmo mode or not and you could use two files to control visibility. If you want to test across module boundary specifically you would generate a swift module.

The example I posted first was what I used for my testing when I added the attribute (I could have split this into two files but it was quicker for me this way). I then made the mistake to assume you were only interested in seeing the attribute working and so I posted the example I had on hand.
In the moment, it did not occur to me that you where also looking at the example for when to use the attribute ...

@aschwaighofer aschwaighofer merged commit ee68d4e into swiftlang:master Mar 5, 2018
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.

6 participants