[ownership] update let properties opts to support ossa #29738
Conversation
|
@gottesmm how do you want to proceed?
But it's probably worth digging in a little more before giving a pass the OSSA badge. I would suggest temporarily modifying the pipeline and gathering results from the benchmarks and also SCK if possible Verify the pipeline is doing what you expect: in both cases assert that all functions are in OSSA at the expect point. Assuming nothing crashes in run 2, count and compare the number of times the optimization kicked in. If they're they same then I'd say this passes muster. |
|
@atrick sounds good to me! I've verified that this works with the buildbot preset (which means the stdlib / benchmarks compile / run). I've updated the pass / pipeline to run the ownership eliminator after this pass and updated the pass to count how many functions it changes and write that to a file. I'm running the buildbot preset again with that change now and I'll run it again after I update the pipeline to add the eliminator before this pass. Then, I can get compare the total functions changed on both (which should be the same, as you said). I'll report back later today after those things have completed.
What do you mean by this? In the first case, the functions should never be in OSSA, right? |
|
This is running significantly later than where ossa is for the stdlib. I have not began that work. So, I am happy if you want to update this to ossa, but it needs to keep working on non-ossa as well. My suggestion is to rather than replacing all the things, introduce ossa versions of each of the tests. |
|
Sounds good. I'll move both these tests to Still in the process of generating the numbers from the comments above. I originally also thought that OSSA wouldn't make it to this part of the pass pipeline but, after I added the ossa elimination pass, I started getting errors in the inliner (because an ossa function was being inlined into a non-ossa function or visa-versa) which leads me to believe that ossa does get to that part of the pipeline. Anyway, I was able to resolve the issues and am re-running the build script now. What are your thoughts on adding an ossa fuzzer? In the near future, these passes won't see much ossa code so, in order to make sure they continue to function (not crash/misscompile) we could run them on a bunch of ossa sil code. If you're interested I can work on a patch. |
|
@zoecarver this pass properly is running on transparent functions that are still in ossa. I would prefer not to optimize those since we are going to inline them. |
|
I'll try to investigate this a bit tomorrow. When building the stdlib/benchmarks 165 functions were changed by this pass. After adding |
|
@gottesmm to clarify, you're asking that I add a check for transparent functions and ignore them? |
|
No, what I am saying is that is most likely the source of why you are seeing ossa issues there. Given that it is transparent functions, I don't think adding ossa support now really matters since we are going to inline those transparent functions most likely anyways. The pathway forward for OSSA is first I am going to move the stdlib to run before serialization at -O with OSSA and then everyone else. This is due to serialization issues. Until that happens you will only see transparent ossa functions in the optimizer pipeline. |
|
@gottesmm alrighty. I see what you're saying. Like I said in my initial comment, if you don't want to proceed with this patch, that's completely OK with me (or wait until after you flip the switch on the stdlib). We could also merge this knowing it won't do anything until later. The ownership eliminator doesn't modify transparent functions so, that's most likely the reason for the difference in the number of functions changed. |
|
My suggestions for testing OSSA migration are based on the idea that we can serialize the stdlib in OSSA form. I thought that was working, at least for experimental purposes, but maybe not? |
|
@atrick We do not serialize it in that form today. Only transparent functions. That being said, I think this patch is fine as long as it bails on transparent functions. I will have to do this work anyways and doing this type of transformation (converting the pass to support OSSA/non-OSSA and adding an ossa version of the test) is the first part of that work I would do anyways. When I get around to moving ownership back through the opt pipeline further towards serialization I will remove the transparent thing. I just want to move ownership lowering backwards pass by pass. |
In subsequent patches, I can help do this by prioritizing passes towards the start of the pass line, if that would be better. |
|
That would be great! |
|
No rush reviewing but, just so I am clear, are there currently any action items for me on this patch? |
|
/cc @gottesmm Hey @zoecarver , Thanks again for doing this! I agree that this first OSSA conversion step is useful and sufficient for merging a PR; namely:
However, the lit tests are extremely weak in terms of coverage of SIL patterns. The majority of work that was historically done in the optimizer never had .sil unit tests. By being just a little more proactive up-front, we can save time debugging random issues in the future. Before declaring a pass "OSSA-compatible". As a second step, I suggest running the pass over the stdlib and benchmarks builds with assertions enabled. There's nothing to actually commit here; this is purely for one-shot testing purposes, but I think it is worth doing and doesn't take long. First, you can try running the pass just after OwnershipEliminator: --- a/lib/SILOptimizer/PassManager/PassPipeline.cpp
Just build the stdlib, overlays and Swift benchmarks--you can ignore the lit unit tests for this experiment. For extra credit, you can insert a counter or trace how many times the optimization kicks in. Assuming that works (it appears to work fine for LetPropertiesOpt), then move the pass to just before the OwnershipModelEliminator. Rebuild the stdlib, overlays, and benchmarks, now with your pass running OSSA. Look for:
You could do this experiment in a WIP PR commit if you want, but, again none of it gets checked in. This is obviously not a perfect experiment, because some optimizations won't kick in until other passes have run first, but I think it's an easy way to catch common issues up front. |
|
@atrick great, thank you for that message. The step-by-step is really helpful. I'm running the build now, as you specified, with the counting code from before. With the exception of #30025 in the future, I'll just be moving back from where we currently stop supporting ownership (the next one is |
|
Actually, one question: what do you mean overlays? Currently, I'm just running |
|
When the pass is after ownership elimination: As you can probably see, there's a difference there. I'll run them again tonight to verify that those numbers are actually the case and if so, I'll try to look into the reason for the difference tomorrow. |
|
@zoecarver without any special build-script options, the stdlib, overlays, and benchmarks are built by default. So just running "ninja" should do it. The overlays are all the .swiftmodules and .dylibs built in swift-macosx-x86_64/lib/swift/macosx, in addition to the stdlib (Swift.swiftmodule/libswiftCore.dylib). The most important probably being Darwin and Foundation. |
|
The discrepancy is strange. You can compare the output of '-Xllvm -debug-only=let-properties-opt' |
|
@atrick thanks for all the help with this. Sorry for the delayed reply, I've been unfortunately busy with other things this week. I think I was cleaning my build targets incorrectly and that is the cause for the difference I saw earlier. Anyway, I just generated the following numbers and function lists. Pass after ownership elimination Changed functionsPass before ownership elimination Changed functionsMore important is this: no assertions or errors :) |
|
I can push the code that used to generate the above data to this PR if you want but like you said because it won't ultimately get committed, I don't think it makes a lot of sense. |
There was a problem hiding this comment.
@zoecarver thanks! I also didn't see any errors when I built the stdlib with LetProperties + OSSA.
You don't need to post the experiment, I just think it's a good sanity check to catch any issues while the code is fresh in our minds.
|
@zoecarver please request commit access if you haven't yet |
|
@swift-ci test |
|
@swift-ci benchmark |
Performance: -OCode size: -OPerformance: -Osize
Code size: -OsizePerformance: -OnoneCode size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@atrick thanks again for the help with this patch. Will do. |
This patch is a bit of an experiment for me. It updates "let-properties-opt" to support OSSA. I realize that this patch will require a lot of work from others (knowledge transfer, etc.). Given that, if you ask me to close this patch, I will happily do that, and I will not take it personally.
With that out of the way, here's the first of what will certainly be many more questions: is this a good pass to have updated? Given that passes before it require non-ossa sil, this pass may not receive ossa sil for a while (other than through sil-opt).
If you want to continue with this patch, I will write many more ossa sil tests.
cc @atrick @gottesmm