> On Jul 18, 2017, at 10:40 AM, Johannes Weiß <johanneswe...@apple.com> wrote:
> 
> Thanks, both suggestions look great. Will work them in tomorrow and will also 
> try to add a test for the whole thing.
> 
>> On 18 Jul 2017, at 5:53 pm, Michael Gottesman <mgottes...@apple.com> wrote:
>> 
>>> 
>>> On Jul 18, 2017, at 8:39 AM, Johannes Weiß <johanneswe...@apple.com> wrote:
>>> 
>>> Hi Erik,
>>> 
>>>> On 17 Jul 2017, at 10:26 pm, Erik Eckstein <eeckst...@apple.com> wrote:
>>>> 
>>>> Hi Johannes,
>>>> 
>>>> great that you want to work on this!
>>> 
>>> Thanks for your help, without Michael's and your help I wouldn't have been 
>>> able to do anything here really!
>>> 
>>> 
>>>> Some ideas:
>>>> SideEffectAnalysis currently does not have a notion of “this argument is 
>>>> not modified by the callee” if the callee is unknown or does anything 
>>>> non-trivial.
>>>> Therefore I think it’s best to put the in_guarantee check directly into 
>>>> MemoryBehaviorVisitor::visitApplyInst():
>>>> If the parameter convention is in_guaranteed and the parameter is V, then 
>>>> the behavior can be MemBehavior::MayRead
>>> 
>>> 
>>> Thanks, that actually makes a lot of sense. Here's my diff right now
>>> 
>>> diff --git a/lib/SILOptimizer/Analysis/MemoryBehavior.cpp 
>>> b/lib/SILOptimizer/Analysis/MemoryBehavior.cpp
>>> index b1fe7fa665..c44cc64f94 100644
>>> --- a/lib/SILOptimizer/Analysis/MemoryBehavior.cpp
>>> +++ b/lib/SILOptimizer/Analysis/MemoryBehavior.cpp
>>> @@ -245,6 +245,23 @@ MemBehavior 
>>> MemoryBehaviorVisitor::visitApplyInst(ApplyInst *AI) {
>>>     (InspectionMode == RetainObserveKind::ObserveRetains &&
>>>      ApplyEffects.mayAllocObjects())) {
>>>   Behavior = MemBehavior::MayHaveSideEffects;

You should move your new code out of the if (ApplyEffects.mayReadRC() ...
Otherwise it will not be executed if the called function does not read a 
reference count.

Beside that it looks good to me.

>>> +
>>> +    unsigned Idx = 0;
>>> +    bool AllReadOnly = false;
>>> +    for (Operand &operand : AI->getArgumentOperands()) {
>>> +        if (operand.get() == V && 
>>> AI->getOrigCalleeType()->getParameters()[Idx].isIndirectInGuaranteed()) {
>>> +            AllReadOnly = true;
>>> +        } else {
>>> +            AllReadOnly = false;
>>> +            break;
>>> +        }
>>> +
>>> +        Idx++;
>>> +    }
>>> +
>>> +    if (AllReadOnly) {
>>> +        Behavior = MemBehavior::MayRead;
>>> +    }
>> 
>> Suggestion:
>> 
>> if (all_of(enumerate(AI->getArgumentOperands()),
>>             [&](std::pair<unsigned, SILValue> pair) -> bool {
>>                return pair.second.get() == V && 
>> AI->getOrigCalleeType()->getParameters()[Idx].isIndirectInGuaranteed()
>>             })) {
>>  Behavior = MemBehavior::MayRead;
>> }
>> 
>> I may have gotten the order of the pair templates wrong. But something like 
>> this is a little cleaner.
>> 
>> Michael
>> 
>>> } else {
>>>   auto &GlobalEffects = ApplyEffects.getGlobalEffects();
>>>   Behavior = GlobalEffects.getMemBehavior(InspectionMode);
>>> 
>>> which indeed turns
>>> 
>>> --- SNIP ---
>>> sil @bar : $@convention(thin) (@in Int) -> () {
>>> bb0(%0 : $*Int):
>>> %value_raw = integer_literal $Builtin.Int64, 42
>>> %value = struct $Int (%value_raw : $Builtin.Int64)
>>> store %value to %0 : $*Int
>>> 
>>> %f_buz = function_ref @buz : $@convention(thin) (@in_guaranteed Int) -> ()
>>> %r1 = apply %f_buz(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>> 
>>> %value_again = load %0 : $*Int
>>> %f_test = function_ref @test : $@convention(thin) (Int) -> ()
>>> %r2 = apply %f_test(%value_again) : $@convention(thin) (Int) -> ()
>>> 
>>> /*
>>> %f_bad = function_ref @bad : $@convention(thin) (@in_guaranteed Int) -> ()
>>> %r3 = apply %f_bad(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>> 
>>> %value_again2 = load %0 : $*Int
>>> %r4 = apply %f_test(%value_again2) : $@convention(thin) (Int) -> ()
>>> */
>>> 
>>> %9999 = tuple()
>>> return %9999 : $()
>>> }
>>> --- SNAP ---
>>> 
>>> into
>>> 
>>> --- SNIP ---
>>> bb0(%0 : $*Int):
>>> %1 = integer_literal $Builtin.Int64, 42         // user: %2
>>> %2 = struct $Int (%1 : $Builtin.Int64)          // users: %7, %3
>>> store %2 to %0 : $*Int                          // id: %3
>>> // function_ref buz
>>> %4 = function_ref @buz : $@convention(thin) (@in_guaranteed Int) -> () // 
>>> user: %5
>>> %5 = apply %4(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>> // function_ref test
>>> %6 = function_ref @test : $@convention(thin) (Int) -> () // user: %7
>>> %7 = apply %6(%2) : $@convention(thin) (Int) -> ()
>>> %8 = tuple ()                                   // user: %9
>>> return %8 : $()                                 // id: %9
>>> } // end sil function 'bar'
>>> --- SNAP ---
>>> 
>>> so the load has successfully been eliminated. Also taking my initial repro 
>>> [1], the retain, the load, and the release are now gone 😃.
>>> 
>>> Is that roughly what you were thinking of?
>>> 
>>> Many thanks,
>>> Johannes
>>> 
>>> [1]: https://bugs.swift.org/secure/attachment/12378/test.swift
>>> 
>>>> 
>>>> Erik
>>>> 
>>>>> On Jul 17, 2017, at 9:23 AM, Johannes Weiß <johanneswe...@apple.com> 
>>>>> wrote:
>>>>> 
>>>>> Thanks Michael!
>>>>> 
>>>>> Erik, I hope what I wrote makes some sense. Any questions or suggestions, 
>>>>> please let me know.
>>>>> 
>>>>>> On 14 Jul 2017, at 7:30 pm, Michael Gottesman <mgottes...@apple.com> 
>>>>>> wrote:
>>>>>> 
>>>>>>> 
>>>>>>> On Jul 12, 2017, at 9:53 AM, Johannes Weiß <johanneswe...@apple.com> 
>>>>>>> wrote:
>>>>>>> 
>>>>>>> Hi Michael,
>>>>>>> 
>>>>>>> 
>>>>>>>> [...]
>>>>>>>>> Long story short, I think the RLE actually works for the test case I 
>>>>>>>>> created. It's even clever enough to see through my invalid function 
>>>>>>>>> bad() which modified the storage despite its claim that it doesn't. I 
>>>>>>>>> might also be misunderstanding something.
>>>>>>>> 
>>>>>>>> When something is marked as in_guaranteed, it should be immutable. If 
>>>>>>>> the callee violates that, then the SIL is malformed. Perhaps, we can 
>>>>>>>> add some sort of verification check.
>>>>>>> 
>>>>>>> Right, yes I was aware that that'd be illegal but I added it as a way 
>>>>>>> to check whether this optimisation is 'looking into' the called 
>>>>>>> function.
>>>>>>> 
>>>>>>> 
>>>>>>>> That being said, I have a good feeling that there is some sort of 
>>>>>>>> analysis occuring here since you provided enough information to the 
>>>>>>>> optimizer. The optimization here is regardless of whether or not we 
>>>>>>>> can see the body of a function, we know that it is safe to optimize 
>>>>>>>> this just based off the @in_guaranteed. This implies using a 
>>>>>>>> declaration, not a definition of the bad function.
>>>>>>> 
>>>>>>> makes sense, didn't think about just only declaring it...
>>>>>>> 
>>>>>>> 
>>>>>>>> When I said write the SIL by hand, what I meant was writing the whole 
>>>>>>>> program by hand. In general, we prefer SIL programs that do not have 
>>>>>>>> extraneous stuff that is added by the compiler (for instance 
>>>>>>>> debug_value). Additionally, it is important for SIL files to not have 
>>>>>>>> dependencies on the stdlib unless absolutely necessary (i.e. your 
>>>>>>>> usage of Int). This prevents the stdlib maintainers from having to 
>>>>>>>> update these tests given chances to the stdlib. Below is a cleaned up 
>>>>>>>> version that shows the problem. Look at how small it is and how it 
>>>>>>>> tests /exactly/ what we are trying to test and via the use of 
>>>>>>>> declarations and the like we are able to exclude other optimizations:
>>>>>>> 
>>>>>>> That makes a lot of sense, thank you. I wasn't yet that familiar with 
>>>>>>> SIL so I thought I start from a program generated by the compiler and 
>>>>>>> then replace the guts with handwritten SIL. But your version is clearly 
>>>>>>> a lot easier to understand and shows what precisely we want to see!
>>>>>>> 
>>>>>>> Today, I looked into why this is happening more precisely.
>>>>>>> 
>>>>>>> So we don't get the RLE because in this code
>>>>>>> 
>>>>>>> if (isComputeAvailValue(Kind) || isPerformingRLE(Kind)) {
>>>>>>> for (unsigned i = 0; i < Locs.size(); ++i) {
>>>>>>> if (isTrackingLocation(ForwardSetIn, Ctx.getLocationBit(Locs[i])))
>>>>>>>   continue;
>>>>>>> updateForwardSetAndValForRead(Ctx, Ctx.getLocationBit(Locs[i]),
>>>>>>>                               Ctx.getValueBit(Vals[i]));
>>>>>>> // We can not perform the forwarding as we are at least missing
>>>>>>> // some pieces of the read location.
>>>>>>> CanForward = false;
>>>>>>> 
>>>>>>> (https://github.com/apple/swift/blob/86620aaa7ebd32d33f4cdf61add5c63a72d3f02a/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp#L917)
>>>>>>> 
>>>>>>> we're not taking the `continue` for the call to `buz()`. The reason why 
>>>>>>> is that here
>>>>>>> 
>>>>>>> if (!AA->mayWriteToMemory(I, R.getBase()))
>>>>>>> continue;
>>>>>>> // MayAlias.
>>>>>>> stopTrackingLocation(ForwardSetIn, i);
>>>>>>> stopTrackingValue(ForwardValIn, i);
>>>>>>> 
>>>>>>> (https://github.com/apple/swift/blob/86620aaa7ebd32d33f4cdf61add5c63a72d3f02a/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp#L972)
>>>>>>> 
>>>>>>> we're not taking the `continue`, ie. `AA->mayWriteToMemory(I, 
>>>>>>> R.getBase())` is true. The reason for that is that the `SILFunction` 
>>>>>>> for `buz` has
>>>>>>> 
>>>>>>> EffectsKindAttr = Unspecified
>>>>>>> 
>>>>>>> which equates to `MayHaveSideEffects`, that's also what 
>>>>>>> `-debug-only=sil-redundant-load-elim,sil-membehavior` outputs:
>>>>>>> 
>>>>>>> GET MEMORY BEHAVIOR FOR:
>>>>>>> %5 = apply %4(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>> %0 = argument of bb0 : $*Int                    // users: %6, %5, %3
>>>>>>> Found apply, returning MayHaveSideEffects
>>>>>>> 
>>>>>>> 
>>>>>>> So where I'm stuck today is that I'm not sure how `EffectsKindAttr` is 
>>>>>>> actually defined. Sure, `$@convention(thin) (@in_guaranteed Int) -> ()` 
>>>>>>> doesn't actually write to the `@in_guaranteed Int` (as that'd be 
>>>>>>> illegal) but it may have other side effects. So I'm not sure if we can 
>>>>>>> just create the function differently if we find only "read-only" kind 
>>>>>>> of parameters. That'd be I think in
>>>>>>> 
>>>>>>> 
>>>>>>> auto *fn = SILMod.createFunction(SILLinkage::Private, Name.str(), Ty,
>>>>>>>                              nullptr, loc, IsNotBare,
>>>>>>>                              IsNotTransparent, IsNotSerialized);
>>>>>>> 
>>>>>>> (https://github.com/apple/swift/blob/ec6fc4d54db95f78ae72dab29734533f709ea2d7/lib/Parse/ParseSIL.cpp#L508
>>>>>>>  -> 
>>>>>>> https://github.com/apple/swift/blob/157db57506b813837481b574a9d38e806bf954b6/lib/SIL/SILModule.cpp#L249)
>>>>>>> 
>>>>>>> which doesn't specify any EffectsAttrKind and therefore it defaults to 
>>>>>>> `Unspecified`.
>>>>>>> 
>>>>>>> 
>>>>>>> Just as a test, I did put a `[readonly]` in `sil @buz : 
>>>>>>> $@convention(thin) (@in_guaranteed Int) -> ()` and as expected 
>>>>>>> everything propagates through correctly and we get a successful RLE.
>>>>>>> 
>>>>>>> So yes, maybe you have some pointers on where to best educate the 
>>>>>>> compiler that the `buz` function won't write to that bit of memory.
>>>>>> 
>>>>>> I have a few ideas of where to put it, but really the person to bring in 
>>>>>> here is Erik. He is the one who wrote the side-effect part of the 
>>>>>> optimizer. Keep in mind he is on vacation right now until next week. So 
>>>>>> I wouldn't expect a response until then.
>>>>>> 
>>>>>> Michael
>>>>>> 
>>>>>>> 
>>>>>>> Many thanks,
>>>>>>> Johannes
>>>>>>> 
>>>>>>>> 
>>>>>>>> ----
>>>>>>>> sil_stage canonical
>>>>>>>> 
>>>>>>>> import Builtin
>>>>>>>> 
>>>>>>>> struct Int {
>>>>>>>> var _value : Builtin.Int64
>>>>>>>> }
>>>>>>>> 
>>>>>>>> sil @test : $@convention(thin) (Int) -> ()
>>>>>>>> sil @buz : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> sil @bad : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> 
>>>>>>>> sil @bar : $@convention(thin) (@in Int) -> () {
>>>>>>>> bb0(%0 : $*Int):
>>>>>>>> %value_raw = integer_literal $Builtin.Int64, 42
>>>>>>>> %value = struct $Int (%value_raw : $Builtin.Int64)
>>>>>>>> store %value to %0 : $*Int
>>>>>>>> 
>>>>>>>> %f_buz = function_ref @buz : $@convention(thin) (@in_guaranteed Int) 
>>>>>>>> -> ()
>>>>>>>> %r1 = apply %f_buz(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> 
>>>>>>>> %value_again = load %0 : $*Int
>>>>>>>> %f_test = function_ref @test : $@convention(thin) (Int) -> ()
>>>>>>>> %r2 = apply %f_test(%value_again) : $@convention(thin) (Int) -> ()
>>>>>>>> 
>>>>>>>> %f_bad = function_ref @bad : $@convention(thin) (@in_guaranteed Int) 
>>>>>>>> -> ()
>>>>>>>> %r3 = apply %f_bad(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> 
>>>>>>>> %value_again2 = load %0 : $*Int
>>>>>>>> %r4 = apply %f_test(%value_again2) : $@convention(thin) (Int) -> ()
>>>>>>>> 
>>>>>>>> %9999 = tuple()
>>>>>>>> return %9999 : $()
>>>>>>>> }
>>>>>>>> ----
>>>>>>>> 
>>>>>>>> When I run this test file through rle, I get:
>>>>>>>> 
>>>>>>>> ----
>>>>>>>> sil_stage canonical
>>>>>>>> 
>>>>>>>> import Builtin
>>>>>>>> import Swift
>>>>>>>> import SwiftShims
>>>>>>>> 
>>>>>>>> struct Int {
>>>>>>>> @sil_stored var _value: Builtin.Int64
>>>>>>>> init(_value: Builtin.Int64)
>>>>>>>> }
>>>>>>>> 
>>>>>>>> // test
>>>>>>>> sil @test : $@convention(thin) (Int) -> ()
>>>>>>>> 
>>>>>>>> // buz
>>>>>>>> sil @buz : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> 
>>>>>>>> // bad
>>>>>>>> sil @bad : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> 
>>>>>>>> // bar
>>>>>>>> sil @bar : $@convention(thin) (@in Int) -> () {
>>>>>>>> // %0                                             // users: %11, %10, 
>>>>>>>> %6, %5, %3
>>>>>>>> bb0(%0 : $*Int):
>>>>>>>> %1 = integer_literal $Builtin.Int64, 42         // user: %2
>>>>>>>> %2 = struct $Int (%1 : $Builtin.Int64)          // user: %3
>>>>>>>> store %2 to %0 : $*Int                          // id: %3
>>>>>>>> // function_ref buz
>>>>>>>> %4 = function_ref @buz : $@convention(thin) (@in_guaranteed Int) -> () 
>>>>>>>> // user: %5
>>>>>>>> %5 = apply %4(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> %6 = load %0 : $*Int                            // user: %8
>>>>>>>> // function_ref test
>>>>>>>> %7 = function_ref @test : $@convention(thin) (Int) -> () // users: 
>>>>>>>> %12, %8
>>>>>>>> %8 = apply %7(%6) : $@convention(thin) (Int) -> ()
>>>>>>>> // function_ref bad
>>>>>>>> %9 = function_ref @bad : $@convention(thin) (@in_guaranteed Int) -> () 
>>>>>>>> // user: %10
>>>>>>>> %10 = apply %9(%0) : $@convention(thin) (@in_guaranteed Int) -> ()
>>>>>>>> %11 = load %0 : $*Int                           // user: %12
>>>>>>>> %12 = apply %7(%11) : $@convention(thin) (Int) -> ()
>>>>>>>> %13 = tuple ()                                  // user: %14
>>>>>>>> return %13 : $()                                // id: %14
>>>>>>>> } // end sil function 'bar'
>>>>>>>> ----
>>>>>>>> 
>>>>>>>> Michael
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Does that all make sense?
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Johannes
>>>>>>>>> <test-load-forwarding.sil><test-load-forwarding.sil-opt>
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Johannes
>>>>>>>>>>> 
>>>>>>>>>>> [1]: https://bugs.swift.org/browse/SR-5403
> 

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

Reply via email to