<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Johannes,<div class=""><br class=""></div><div class="">great that you want to work on this!</div><div class=""><br class=""></div><div class="">Some ideas:</div><div class="">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.</div><div class="">Therefore I think it’s best to put the in_guarantee check directly into <span style="color: rgb(79, 129, 135); font-family: Menlo; font-size: 11px; background-color: rgb(255, 255, 255);" class="">MemoryBehaviorVisitor</span><span style="font-family: Menlo; font-size: 11px; background-color: rgb(255, 255, 255);" class="">::visitApplyInst()</span>:</div><div class="">If the parameter convention is in_guaranteed and the parameter is V, then the behavior can be <span style="color: rgb(79, 129, 135); font-family: Menlo; font-size: 11px; background-color: rgb(255, 255, 255);" class="">MemBehavior</span><span style="font-family: Menlo; font-size: 11px; background-color: rgb(255, 255, 255);" class="">::</span><span style="font-family: Menlo; font-size: 11px; background-color: rgb(255, 255, 255); color: rgb(49, 89, 93);" class="">MayRead</span></div><div class=""><br class=""></div><div class="">Erik</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 17, 2017, at 9:23 AM, Johannes Weiß <<a href="mailto:johannesweiss@apple.com" class="">johannesweiss@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Thanks Michael!<br class=""><br class="">Erik, I hope what I wrote makes some sense. Any questions or suggestions, please let me know.<br class=""><br class=""><blockquote type="cite" class="">On 14 Jul 2017, at 7:30 pm, Michael Gottesman <<a href="mailto:mgottesman@apple.com" class="">mgottesman@apple.com</a>> wrote:<br class=""><br class=""><blockquote type="cite" class=""><br class="">On Jul 12, 2017, at 9:53 AM, Johannes Weiß <<a href="mailto:johannesweiss@apple.com" class="">johannesweiss@apple.com</a>> wrote:<br class=""><br class="">Hi Michael,<br class=""><br class=""><br class=""><blockquote type="cite" class="">[...]<br class=""><blockquote type="cite" class="">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.<br class=""></blockquote><br class="">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.<br class=""></blockquote><br class="">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.<br class=""><br class=""><br class=""><blockquote type="cite" class="">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.<br class=""></blockquote><br class="">makes sense, didn't think about just only declaring it...<br class=""><br class=""><br class=""><blockquote type="cite" class="">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:<br class=""></blockquote><br class="">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!<br class=""><br class="">Today, I looked into why this is happening more precisely.<br class=""><br class="">So we don't get the RLE because in this code<br class=""><br class="">if (isComputeAvailValue(Kind) || isPerformingRLE(Kind)) {<br class=""> for (unsigned i = 0; i < Locs.size(); ++i) {<br class=""> if (isTrackingLocation(ForwardSetIn, Ctx.getLocationBit(Locs[i])))<br class=""> continue;<br class=""> updateForwardSetAndValForRead(Ctx, Ctx.getLocationBit(Locs[i]),<br class=""> Ctx.getValueBit(Vals[i]));<br class=""> // We can not perform the forwarding as we are at least missing<br class=""> // some pieces of the read location.<br class=""> CanForward = false;<br class=""><br class="">(<a href="https://github.com/apple/swift/blob/86620aaa7ebd32d33f4cdf61add5c63a72d3f02a/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp#L917" class="">https://github.com/apple/swift/blob/86620aaa7ebd32d33f4cdf61add5c63a72d3f02a/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp#L917</a>)<br class=""><br class="">we're not taking the `continue` for the call to `buz()`. The reason why is that here<br class=""><br class=""> if (!AA->mayWriteToMemory(I, R.getBase()))<br class=""> continue;<br class=""> // MayAlias.<br class=""> stopTrackingLocation(ForwardSetIn, i);<br class=""> stopTrackingValue(ForwardValIn, i);<br class=""><br class="">(<a href="https://github.com/apple/swift/blob/86620aaa7ebd32d33f4cdf61add5c63a72d3f02a/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp#L972" class="">https://github.com/apple/swift/blob/86620aaa7ebd32d33f4cdf61add5c63a72d3f02a/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp#L972</a>)<br class=""><br class="">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<br class=""><br class=""> EffectsKindAttr = Unspecified<br class=""><br class="">which equates to `MayHaveSideEffects`, that's also what `-debug-only=sil-redundant-load-elim,sil-membehavior` outputs:<br class=""><br class="">GET MEMORY BEHAVIOR FOR:<br class=""> %5 = apply %4(%0) : $@convention(thin) (@in_guaranteed Int) -> ()<br class=""> %0 = argument of bb0 : $*Int // users: %6, %5, %3<br class="">Found apply, returning MayHaveSideEffects<br class=""><br class=""><br class="">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<br class=""><br class=""><br class="">auto *fn = SILMod.createFunction(SILLinkage::Private, Name.str(), Ty,<br class=""> nullptr, loc, IsNotBare,<br class=""> IsNotTransparent, IsNotSerialized);<br class=""><br class="">(<a href="https://github.com/apple/swift/blob/ec6fc4d54db95f78ae72dab29734533f709ea2d7/lib/Parse/ParseSIL.cpp#L508" class="">https://github.com/apple/swift/blob/ec6fc4d54db95f78ae72dab29734533f709ea2d7/lib/Parse/ParseSIL.cpp#L508</a> -> <a href="https://github.com/apple/swift/blob/157db57506b813837481b574a9d38e806bf954b6/lib/SIL/SILModule.cpp#L249" class="">https://github.com/apple/swift/blob/157db57506b813837481b574a9d38e806bf954b6/lib/SIL/SILModule.cpp#L249</a>)<br class=""><br class="">which doesn't specify any EffectsAttrKind and therefore it defaults to `Unspecified`.<br class=""><br class=""><br class="">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.<br class=""><br class="">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.<br class=""></blockquote><br class="">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.<br class=""><br class="">Michael<br class=""><br class=""><blockquote type="cite" class=""><br class="">Many thanks,<br class="">Johannes<br class=""><br class=""><blockquote type="cite" class=""><br class="">----<br class="">sil_stage canonical<br class=""><br class="">import Builtin<br class=""><br class="">struct Int {<br class="">var _value : Builtin.Int64<br class="">}<br class=""><br class="">sil @test : $@convention(thin) (Int) -> ()<br class="">sil @buz : $@convention(thin) (@in_guaranteed Int) -> ()<br class="">sil @bad : $@convention(thin) (@in_guaranteed Int) -> ()<br class=""><br class="">sil @bar : $@convention(thin) (@in Int) -> () {<br class="">bb0(%0 : $*Int):<br class="">%value_raw = integer_literal $Builtin.Int64, 42<br class="">%value = struct $Int (%value_raw : $Builtin.Int64)<br class="">store %value to %0 : $*Int<br class=""><br class="">%f_buz = function_ref @buz : $@convention(thin) (@in_guaranteed Int) -> ()<br class="">%r1 = apply %f_buz(%0) : $@convention(thin) (@in_guaranteed Int) -> ()<br class=""><br class="">%value_again = load %0 : $*Int<br class="">%f_test = function_ref @test : $@convention(thin) (Int) -> ()<br class="">%r2 = apply %f_test(%value_again) : $@convention(thin) (Int) -> ()<br class=""><br class="">%f_bad = function_ref @bad : $@convention(thin) (@in_guaranteed Int) -> ()<br class="">%r3 = apply %f_bad(%0) : $@convention(thin) (@in_guaranteed Int) -> ()<br class=""><br class="">%value_again2 = load %0 : $*Int<br class="">%r4 = apply %f_test(%value_again2) : $@convention(thin) (Int) -> ()<br class=""><br class="">%9999 = tuple()<br class="">return %9999 : $()<br class="">}<br class="">----<br class=""><br class="">When I run this test file through rle, I get:<br class=""><br class="">----<br class="">sil_stage canonical<br class=""><br class="">import Builtin<br class="">import Swift<br class="">import SwiftShims<br class=""><br class="">struct Int {<br class="">@sil_stored var _value: Builtin.Int64<br class="">init(_value: Builtin.Int64)<br class="">}<br class=""><br class="">// test<br class="">sil @test : $@convention(thin) (Int) -> ()<br class=""><br class="">// buz<br class="">sil @buz : $@convention(thin) (@in_guaranteed Int) -> ()<br class=""><br class="">// bad<br class="">sil @bad : $@convention(thin) (@in_guaranteed Int) -> ()<br class=""><br class="">// bar<br class="">sil @bar : $@convention(thin) (@in Int) -> () {<br class="">// %0 // users: %11, %10, %6, %5, %3<br class="">bb0(%0 : $*Int):<br class="">%1 = integer_literal $Builtin.Int64, 42 // user: %2<br class="">%2 = struct $Int (%1 : $Builtin.Int64) // user: %3<br class="">store %2 to %0 : $*Int // id: %3<br class="">// function_ref buz<br class="">%4 = function_ref @buz : $@convention(thin) (@in_guaranteed Int) -> () // user: %5<br class="">%5 = apply %4(%0) : $@convention(thin) (@in_guaranteed Int) -> ()<br class="">%6 = load %0 : $*Int // user: %8<br class="">// function_ref test<br class="">%7 = function_ref @test : $@convention(thin) (Int) -> () // users: %12, %8<br class="">%8 = apply %7(%6) : $@convention(thin) (Int) -> ()<br class="">// function_ref bad<br class="">%9 = function_ref @bad : $@convention(thin) (@in_guaranteed Int) -> () // user: %10<br class="">%10 = apply %9(%0) : $@convention(thin) (@in_guaranteed Int) -> ()<br class="">%11 = load %0 : $*Int // user: %12<br class="">%12 = apply %7(%11) : $@convention(thin) (Int) -> ()<br class="">%13 = tuple () // user: %14<br class="">return %13 : $() // id: %14<br class="">} // end sil function 'bar'<br class="">----<br class=""><br class="">Michael<br class=""><br class=""><blockquote type="cite" class=""><br class="">Does that all make sense?<br class=""><br class="">Thanks,<br class="">Johannes<br class=""><test-load-forwarding.sil><test-load-forwarding.sil-opt><br class=""><br class=""><br class=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" class=""><br class="">Thanks,<br class="">Johannes<br class=""><br class="">[1]: <a href="https://bugs.swift.org/browse/SR-5403" class="">https://bugs.swift.org/browse/SR-5403</a><br class=""></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote><br class=""></div></div></blockquote></div><br class=""></div></body></html>