[Coverage] Refactor SIL generation for profiling #13597
Conversation
|
@swift-ci please smoke test |
|
Hey @slavapestov and @shajrawi, mind taking a look (possibly after the winter break :)? |
|
@swift-ci please smoke test |
|
This looks good. I'm curious why you decided to put the profiler instance in SILFunction and not SILGenFunction though -- presumably after SILGen we never need to create a profiler instance again, unless you think the optimizer will need to bump these counts too? |
|
LGTM! the profiler instance should be in SILFunction: the optimizer might bump / change profile counts. There's initial support in the Optimizer for profile-based inlining and propagating a profile through other optimizations (which might split a BB / unroll a loop ..etc). While the current support is partial / just a proof of concept, we might want to get back to PGO at a later date. If we do get back to PGO, being able to bump these counts in the optimizer would be extremely useful. |
|
Sorry for the delay, I'm just getting back online 😅. @slavapestov, I put the profiler instance in SILFunction for the reason Joe mentioned. It seems like this is in good shape, so I'll rebase and merge today. Thanks for taking a look! |
This patch moves the ownership of profiling state from SILGenProfiling to SILFunction, where it always belonged. Similarly, it moves ownership of the profile reader from SILGenModule to SILModule. The refactor sets us up to fix a few outstanding code coverage bugs and does away with sad hacks like ProfilerRAII. It also allows us to locally guarantee that a profile counter increment actually corresponds to the SILFunction at hand. That local guarantee causes a bugfix to accidentally fall out of this refactor: we now set up the profiling state for delayed functions correctly. Previously, we would set up a ProfilerRAII for the delayed function, but its counter increment would never be emitted :(. This fix constitutes the only functional change in this patch -- the rest is NFC. As a follow-up, I plan on removing some dead code in the profiling logic and fixing a few naming inconsistencies. I've left that for later to keep this patch simple.
|
@swift-ci please smoke test and merge |
|
@swift-ci Please smoke test |
This patch moves the ownership of profiling state from SILGenProfiling
to SILFunction, where it always belonged. Similarly, it moves ownership
of the profile reader from SILGenModule to SILModule.
The refactor sets us up to fix a few outstanding code coverage bugs and
does away with sad hacks like ProfilerRAII. It also allows us to locally
guarantee that a profile counter increment actually corresponds to the
SILFunction at hand.
That local guarantee causes a bugfix to accidentally fall out of this
refactor: we now set up the profiling state for delayed functions
correctly. Previously, we would set up a ProfilerRAII for the delayed
function, but its counter increment would never be emitted :(. This fix
constitutes the only functional change in this patch -- the rest is NFC.
As a follow-up, I plan on removing some dead code in the profiling
logic and fixing a few naming inconsistencies. I've left that for later
to keep this patch simple.
rdar://32588741 (& 36061003)