Question about why retain&release on @guaranteed

Development Compiler
1

Hi,

I've got a quick question about why swiftc wants a retain/release around a @guaranteed call on an object: I have this demo program:

final class Test {
   private var next: Test? = nil

   func fire() {
       self.next?.fire()
   }
}

and for the fire() function I'd expect

  • a dereference of self to get hold of self.next
  • a branch to see if next == nil
  • if not nil: a direct call to Test.fire(next) passing on the next pointer

However the SIL (from swiftc -frontend -emit-sil -O test.swift) looks like this:

// Test.fire()
sil @test.Test.fire() -> () : $@convention(method) (@guaranteed Test) -> () {
bb0(%0 : $Test):
 %2 = ref_element_addr %0 : $Test, #Test.next
 %3 = load %2 : $*Optional<Test>
 switch_enum %3 : $Optional<Test>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
bb1:
 br bb3
bb2:
 %6 = unchecked_enum_data %3 : $Optional<Test>, #Optional.some!enumelt.1
 %7 = function_ref @test.Test.fire() -> () : $@convention(method) (@guaranteed Test) -> ()
 strong_retain %6 : $Test                 /* UNEXPECTED! why is this necessary? */
 %9 = apply %7(%6) : $@convention(method) (@guaranteed Test) -> ()
 release_value %3 : $Optional<Test>       /* UNEXPECTED! why is this necessary? */
 br bb3

so it all totally makes sense but the retain/release I wouldn't expect. Is this just to guarantee isKnownUniquelyReferenced(...) is false?

Many thanks,
Johannes
PS: Toolchain is swift-4.1-DEVELOPMENT-SNAPSHOT-2018-01-10-a.xctoolchain

1 Like
2

The issue here is that we have turned off the optimization that would trigger here since in the general case in the context of potentially mismatched retain/release pairs, this is unsafe. Making such a transformation safe is part of the work of Semantic SIL.

Now that being said, I wonder if in such small cases like this, we could peephole. I would have to think about it. You would have to be able to guarantee that there aren't any retains/releases in between the retain/release that either of the instructions could be paired with.

3

Would it be difficult to change SILGen to not emit the unnecessary retain/release in the first place? It knows the parameter is guaranteed, and I suspect it converts it to a +1 value needlessly.

Slava

2 Likes
4

Feel free to try.

5

@Michael_Gottesman/@Slava_Pestov should I file a SR for this missed optimisation opportunity?

6

The retain is necessary because fire() could end up modifying self.next. There's not really a better answer here without a much smarter effects system.

1 Like
7

Ooops. You are right. I read it incorrectly. We /could/ do this if the property was a let though.

8

Oops, yes, I totally missed that, thanks @jrose for pointing that out! To express what I want which is 'I know this will be alive when this is called' I would need @Erik_Eckstein's SE draft swift-evolution/0000-unsafe-guaranteed.md at unmanaged_unsafe_guaranteed_proposal · aschwaighofer/swift-evolution · GitHub to be implemented and then I could write

var next: Unmanaged<Test>

func fire() {
    self.next.withUnsafeGuaranteedValue {
        $0.fire()
    }
}

or is there any other way to express this that works right now?

9

Ha, so turns out this actually works today with the unsupported _withUnsafeGuaranteedRef, tempting...

public class Test {
    private var next: Unmanaged<Test>

    public func fire() {
        self.next._withUnsafeGuaranteedRef {
            $0.fire()
        }
    }
}

becomes:

// Test.fire()
sil @_T04test4TestC4fireyyF : $@convention(method) (@guaranteed Test) -> () {
// %0                                             // users: %2, %1
bb0(%0 : $Test):
  debug_value %0 : $Test, let, name "self", argno 1 // id: %1
  %2 = ref_element_addr %0 : $Test, #Test.next    // user: %3
  %3 = struct_element_addr %2 : $*Unmanaged<Test>, #Unmanaged._value // user: %4
  %4 = load %3 : $*@sil_unmanaged Test            // user: %5
  %5 = unmanaged_to_ref %4 : $@sil_unmanaged Test to $Test // user: %7
  // function_ref Test.fire()
  %6 = function_ref @_T04test4TestC4fireyyF : $@convention(method) (@guaranteed Test) -> () // user: %7
  %7 = apply %6(%5) : $@convention(method) (@guaranteed Test) -> ()
  %8 = tuple ()                                   // user: %9
  return %8 : $()                                 // id: %9
} // end sil function '_T04test4TestC4fireyyF'

Any chance we could see this without the underscore soon?

10

My hope is that when semantic sil is done, you will not need such support in the general case. That being said, I don't remember all of the use cases for this and the restrictions. I would ask Arnold. But that being said I am not 100% sure what his forum name is (@Arnold)?

11

I just imed the URL to him, so we will see what it is.

1 Like
12

Let see. Yeah that seems to be me.

13

I don't think this is going to see the light of day in a supported form (without the underscore).

Michael is right that the need for this should mostly be subsumed by optimizing semantic sil.

The constraints are are outlined in the proposal. The user of _withUnsafeGuaranteedRef needs to ensure that there is another reference keeping next alive for the duration of the _withUnsafeGuaranteedRef call. The code you outlined is unsafe without further knowledge. self: Test lifetime is not guaranteed across the call. There is nothing that prevents the optimizer from moving the potentially final release of self before the withUnsafeGuaranteed call. Something like _fixlifetime has to ensure safety.

    public func fire() {
        self.next._withUnsafeGuaranteedRef {
            $0.fire()
        }
        _fixLifetime(self)
    }
14

Thanks @Arnold, I'm aware that I will need to guarantee that there's another reference keeping the object _withUnsafeGuaranteedRef is called on alive (sorry, my example was quite incomplete). In my actual application that is definitely ensured so I believe that should be safe.

@Michael_Gottesman what I can't quite follow yet is how with semantic SIL done we could proof this automatically. Let's assume I have

public final class Node {
    internal var next: Node?
    internal var prev: Node?

    public func fire() {
        [...]
        self.next?.fire()
    }
}

So essentially I have a doubly linked list of Nodes. Now in my application I know, that the linked list never gets modified in the fire calls. Therefore I know that ref-counting self.next isn't necessary. That's exactly what I wanted to express with the _withUnsafeGuaranteedRef. Would semantic SIL done really be able to elide this ref-counting by itself, ie. without using _withUnsafeGuaranteedRef?

15

Yes. Given that you are dealing with a final in module thing, our analysis will know that the value is not being written to. In such a case, we would be able to change the load [copy] to a load borrow. I think you would need some extra constraints (I am hand waving a little bit), but that is the general gist.

3 Likes
16

thanks @Michael_Gottesman, can't wait until that will be possible :raised_hands:

17

Me too... me too. = ).

1 Like