Break some unfortunate dependencies in jitc_yk #1545
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit is a bit of a hack, but it starts to make it possible for us to have more than one codegen active at any one time. In order to do that, we have to make the `VarLocation` struct not hardcode in x64 registers. That means making `VarLocation` take a type parameter for the particular codegen's register type. However, doing that causes a proliferation of typing errors across the codebase. Fixing those would not be fun, and a quick experiment suggests wouldn't even be a good idea: they just make a lot of the code harder to read. Instead, what this commit does is use a couple of strategically placed `typedef`s of the form: ``` typedef VarLocation = reg_alloc::VarLocation<Register>; ``` That means that referring to just `VarLocation` is correct at nearly all the points it ever was. Note: this commit does not finish off the de-x64isation of jitc_yk, but it does make the future path to doing so, and probable solutions, clearer.
|
This looks fine, apart from the typo in the second commit title. Feel free to force push a fix. |
This commit is, again, a bit of a hack, but it still makes things a bit better than before. In essence, before this commit JIT modules contained `yksmp::Location`s for "normal" traces and `VarLocation`s for side traces. The former are codegen agnostic: the latter are codegen specific. This commit hoiks the `VarLocation`s out of the JIT mod and puts them in `TraceKind::Sidetrace`. Now that would end up having us add a `Register` type parameter everywhere, which is too much churn, so for now `TraceKind::Sidetrace` stores an `Arc<SidetraceInfo>` i.e. we store it as a trait object. We then have to downcast to `YkSideTraceInfo` in lots of places. This is pretty evil, because we're using dynamic typing where we should be using static typing. However, overall, this is the lesser of the evils. By the end of this commit, it's clearer (even if not perfectly so) the relationship between trace inputs and side-trace outputs. We can do better in the future, and maybe we'll want to do so, but this does feel like a nudge in the right direction.
ltratt
force-pushed
the
varlocation_generic
branch
from
170c2f1 to
a7050e7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In essence this PR -- which will be a bit hard to review -- breaks some unfortunate dependencies in jitc_yk:
VarLocationwas specific to our x86 backend. b411c0e largely hacks that out.jir_ir::Modulewas also -- though it wasn't obvious! -- also x86 backend specific because of side-traces. 170c2f1 hacks that out.Neither of these commits ends us up with a perfect design, but overall this does nudge us towards something that makes other changes easier to understand IMHO.