Thanks very much Eric & Michael for your help! And thanks for merging it. Will this automatically be part of Swift 4 or do I need to make another PR against a swift 4 branch?
Thanks, Johannes > On 18 Jul 2017, at 9:21 pm, Johannes Weiß via swift-dev <swift-dev@swift.org> > wrote: > > 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 _______________________________________________ swift-dev mailing list swift-dev@swift.org https://lists.swift.org/mailman/listinfo/swift-dev
