irinterp: improve semi-concrete interpretation accuracy #47994
Conversation
|
@nanosoldier |
|
Not really a huge fan of this approach. Can we just stop widening there? That's done for the benefit of codegen, but we've moved both |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
aviatesk
force-pushed
the
avi/improve-semi-concrete-accuracy
branch
from
678e2ad
to
67d9dc5
Compare
es, I just implemented your suggestion. We may want to switch back to my first approach once our system becomes able to run code produced by an external |
|
@nanosoldier |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
aviatesk
force-pushed
the
avi/improve-semi-concrete-accuracy
branch
2 times, most recently
from
a1d9f21
to
cdbab61
Compare
src/interpreter.c
Outdated
| @@ -210,7 +210,7 @@ static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s) | |||
| jl_value_t *val = eval_value(jl_fieldref_noalloc(e, 0), s); | |||
| #ifndef JL_NDEBUG | |||
| JL_GC_PUSH1(&val); | |||
| jl_typeassert(val, jl_fieldref_noalloc(e, 1)); | |||
| jl_typeassert(val, jl_widen_core_extended_info(jl_fieldref_noalloc(e, 1))); | |||
There was a problem hiding this comment.
analyzegc complains on this line. Could someone tell me how I can avoid it?
There was a problem hiding this comment.
I didn't look at the error message, but I assume the issue is that jl_widen_core_extended_info can return a freshly-allocated result in the Type case, which isn't rooted anywhere. You can fix that by just changing the JL_GC_PUSH1 to JL_GC_PUSH2 above (on some other variable jl_value_t *typ = NULL (note the value at PUSH time matters, it needs to always be a valid pointer or null between the push and the pop)). Then just assign the result of the widening to that variable and it'll be rooted for the call.
There was a problem hiding this comment.
Thanks so much for your help! That seems to work.
aviatesk
force-pushed
the
avi/improve-semi-concrete-accuracy
branch
2 times, most recently
from
523db51
to
0d71680
Compare
aviatesk
force-pushed
the
avi/improve-semi-concrete-accuracy
branch
from
0d71680
to
830cdb6
Compare
Currently semi-concrete interpretation can end up with more inaccurate result than usual abstract interpretation because `src.ssavaluetypes` are all widened when cached so semi-concrete interpretation can't use extended lattice information of `SSAValue`s. This commit tries to fix it by making `CodeInstance` keep extended lattice information of `SSAValue`s that are widened and allowing semi-concrete interpretation to override `src.ssavaluetypes` when it uncompressed `src` from `CodeInstance`. I found there are other chances when semi-concrete interpretation can end up with inaccurate results, but that is unrelated to this and should be fixed separately.
This reverts commit 67d9dc5.
Currently semi-concrete interpretation can end up with more inaccurate result than usual abstract interpretation because `src.ssavaluetypes` are all widened when cached so semi-concrete interpretation can't use extended lattice information of `SSAValue`s. This commit tries to fix it by making the codegen system recognize extended lattice information such as `Const` and `PartialStruct` and not-widening those information from `src::CodeInfo` when caching. I found there are other chances when semi-concrete interpretation can end up with inaccurate results, but that is unrelated to this and should be fixed separately.
aviatesk
force-pushed
the
avi/improve-semi-concrete-accuracy
branch
from
830cdb6
to
7ace688
Compare
|
@nanosoldier |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
|
We primarily used to do this to avoid exploding the precompile caches with garbage data that wastes space and costs GC time permanently. Are we sure this PR has not introduced a regression in that area, since we are recently starting to see test failures from an excess of roots being created (e.g. #48089)? |
|
I haven't checked anything in that area yet. Maybe I should check images size difference first, or is there any other recommended way to check the regression? |
|
You could instrument ircode.c to make sure there are no new objects getting created and stored as roots |
…crete-accuracy" (#48194) This reverts commit 03bdf15, reversing changes made to a9e0545. Fixes #48089 regression That PR causes the runtime to store an undesirable amount of garbage, to work around a bug in the semi-concrete interpreter failing to correctly compute the inferred result. That should be fixed in the semi-concrete interpreter, without bloating the runtime caches with extraneous information.
Currently semi-concrete interpretation can end up with more inaccurate result than usual abstract interpretation because
src.ssavaluetypesare all widened when cached so semi-concrete interpretation can't use extended lattice information ofSSAValues. This commit tries to fix it by makingCodeInstancekeep extended lattice information ofSSAValues that are widened and allowing semi-concrete interpretation to overridesrc.ssavaluetypeswhen it uncompressedsrcfromCodeInstance.I found there are other chances when semi-concrete interpretation can end up with inaccurate results, but that is unrelated to this and should be fixed separately.