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;
>> +
>> +    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