ClosureSpecialization: support for OSSA and a big overhaul #84704
Conversation
* `var UncheckedValueCastInst.fromValue` * `BeginApplyInst.isNonThrowing` and `BeginApplyInst.isNonAsync` * `Builder.createUncheckedValueCast`
…hen creating a new entry block in the cloned function This allows clients to directly clone specific instructions into the new entry block
…rovided the cloned entry block
…tution maps When mangling types, it's expected that the type is canonical. Except if mangling for debug info. Fixes a crash.
…pecialize for the same closure as a previous argument
For example:
```
%1 = partial_apply %closure
apply %f(%1, %1) // first argument: `.closure(%1)`
// second argument: `.previousArgumentIndex(0)`
```
eeckstein
requested review from
artemcm,
asl,
hborla,
jckarter,
rjmccall,
slavapestov,
tshortli and
xedin
as code owners
eeckstein
requested review from
aidan-hall,
atrick,
kovdan01,
meg-gupta and
nate-chandler
and removed request for
artemcm,
hborla,
jckarter,
rjmccall,
slavapestov,
tshortli and
xedin
|
@swift-ci smoke test |
Beside supporting OSSA, this change significantly simplifies the pass. The main change is that instead of starting at a closure (e.g. `partial_apply`) and finding all call sites, we now start at a call site and look for closures for all arguments. This makes a lot of things much simpler, e.g. not so many intermediate data structures are required to track all the states. I needed to remove the 3 unit tests because the things those tests were testing are not there anymore. However, the pass is tested with a lot of sil tests (and I added quite a few), which should give good test coverage. The old ClosureSpecializer pass is still kept in place, because at that point in the pipeline we don't have OSSA, yet. Once we have that, we can replace the old pass withe the new one. However, the autodiff closure specializer already runs in the OSSA pipeline and there the new changes take effect.
eeckstein
force-pushed
the
closure-specialization
branch
from
cdb3783 to
8efafc7
Compare
|
@swift-ci smoke test |
In swiftlang#84704, some tests were XFAIL'ed since they've become failing. The root cause of failures is lack of ownership info on the 2nd run of AutoDiff closure specialization pass. Ownership info is required for the pass run after swiftlang#84704, so at the moment only the 1st run of the pass is effective. Several test cases still remain passing because for some cases we are lucky and all the inlining required for specialization is done before the 1st pass run. This patch enables such test cases back so we have at least some test coverage.
|
@eeckstein As far as I can see, the 2nd run of AutoDiff closure specialization (ADCS) pass is now effectively useless because it runs at the point when we have no ownership info, and the ADCS is now only effective during its first run. Unfortunately, I was unable to re-order the pass pipeline to make 2nd attempt of ADCS effective - it looks like that we either have ownership info already lost or have some inlining required not happened yet. XFAIL'ed tests clearly show us that we loose optimization opportunities which we were able to use before. Disabling ADCS would drastically drop the performance - we've already faced that before when ADCS was temporarily disabled as part of #78849 If I get your in-line comments in code and the PR description correct, we'll eventually have OSSA during "generic" closure specialization, which would also mean that we'll have OSSA during 2nd run of ADCS. Could you please clarify if getting OSSA at that point is a matter of weeks/months/...years?.. If you have plans to have OSSA present at that point relatively soon, we could probably just wait and be happy, while leaving 2nd run of ADCS not working between releases (if we assume that we'll get it back working "for free" before the next release just by having OSSA at the corresponding point in pipeline before the next release). But, if achieving this would take longer time, I might probably need to make ADCS agnostic to OSSA so we do not lose performance here. Would be glad if you could share your thoughts on the problem. Also tagging @JaapWijnen |
right
It's planned for the next release and I'm actively working on it (work in progress: https://github.com/eeckstein/swift/tree/ossa). So I'm kindly asking for you patience 🙂
That's not worth the effort. |
@eeckstein Thanks! It's a very good piece of news and we're looking forward to this :) It would be nice if you could tag me when the corresponding PR is opened. |
In #84704, some tests were XFAIL'ed since they've become failing. The root cause of failures is lack of ownership info on the 2nd run of AutoDiff closure specialization pass. Ownership info is required for the pass run after #84704, so at the moment only the 1st run of the pass is effective. Several test cases still remain passing because for some cases we are lucky and all the inlining required for specialization is done before the 1st pass run. This patch enables such test cases back so we have at least some test coverage.
In swiftlang#84704, some tests were XFAIL'ed since they've become failing. The root cause of failures is lack of ownership info on the 2nd run of AutoDiff closure specialization pass. Ownership info is required for the pass run after swiftlang#84704, so at the moment only the 1st run of the pass is effective. Several test cases still remain passing because for some cases we are lucky and all the inlining required for specialization is done before the 1st pass run. This patch enables such test cases back so we have at least some test coverage.
In swiftlang#84704, some tests were XFAIL'ed since they've become failing. The root cause of failures is lack of ownership info on the 2nd run of AutoDiff closure specialization pass. Ownership info is required for the pass run after swiftlang#84704, so at the moment only the 1st run of the pass is effective. Several test cases still remain passing because for some cases we are lucky and all the inlining required for specialization is done before the 1st pass run. This patch enables such test cases back so we have at least some test coverage.
2 participants
Beside supporting OSSA, this change significantly simplifies the pass.
The main change is that instead of starting at a closure (e.g.
partial_apply) and finding all call sites, we now start at a call site and look for closures for all arguments. This makes a lot of things much simpler, e.g. not so many intermediate data structures are required to track all the states.I needed to remove the 3 unit tests because the things those tests were testing are not there anymore. However, the pass is tested with a lot of sil tests (and I added quite a few), which should give good test coverage.
The old ClosureSpecializer pass is still kept in place, because at that point in the pipeline we don't have OSSA, yet. Once we have that, we can replace the old pass withe the new one.
However, the autodiff closure specializer already runs in the OSSA pipeline and there the new changes take effect.