> 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