great, thank you. I think I have worked all the suggestions in, let's do the
rest in this PR: https://github.com/apple/swift/pull/11040
It's getting late here so I'll probably to the test tomorrow (but marked the
patch as 'do not merge' & added that a test is still missing).
> On 18 Jul 2017, at 7:29 pm, Erik Eckstein <eeckst...@apple.com> wrote:
>
>>
>> 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
