Make a start on the JIT IR and the trace builder. #930
Conversation
ykrt/src/compile/jitc_yk/jit_ir.rs
Outdated
|
|
||
| impl Operand { | ||
| pub(crate) fn new(kind: OpKind, val: u64) -> Self { | ||
| if val < 2u64.pow(15) { |
There was a problem hiding this comment.
Hang on, isn't this exceeding the range of u64? What is the aim here? I wonder if what was intended was to reserve u64::MAX as a special value?
There was a problem hiding this comment.
It's checking if the operand's payload can fit in a small operand, where we have only 15-bits of wiggle room.
(a long instruction, once implemented, would allow larger operands)
I missed the 15 in my "kill all magic numbers" pass too.
There was a problem hiding this comment.
(2^15 should fit in a u64 though, or I've gone mad)
There was a problem hiding this comment.
Right, got it. I'll leave the comment here so we remember to resolve it.
There was a problem hiding this comment.
(2^15 should fit in a u64 though, or I've gone mad)
I misread it because I'm so unfamiliar with that way of doing bit patterns -- mea culpa!
There was a problem hiding this comment.
Added a constant in a34b500
|
@ltratt last two commits ok for you? |
| @@ -89,7 +89,8 @@ pub enum Operand { | |||
|
|
|||
| impl Operand { | |||
| pub(crate) fn new(kind: OpKind, val: u64) -> Self { | |||
| if val < 2u64.pow(u32::try_from(SHORT_OPERAND_VALUE_SIZE).unwrap()) { | |||
| // check if the operand's value can fit in a short operand. | |||
| if val & (u64::MAX << SHORT_OPERAND_VALUE_SIZE) == 0 { | |||
There was a problem hiding this comment.
It's still not obvious to me what SIZE is (OK, OK, I know now, but I'll forget later, because it's unclear). Can we put all the maths in the constant at the top of the file when it will become much clearer?
There was a problem hiding this comment.
Did eb47831 not work?
There was a problem hiding this comment.
OK the docs on SIZE help, but let's still stick all the maths in the constant (if nothing else, it makes clearer a) what the values actually are b) they're compile-time constants).
There was a problem hiding this comment.
I think that would need to be another PR as there are lots of computations we'd want to move to the top.
There was a problem hiding this comment.
All of them are introduced in this PR aren't they? Or are they preexisting?
There was a problem hiding this comment.
They are, yes, but if you want them in this PR then it will have to be next year now.
There was a problem hiding this comment.
OK, promise me you'll fix it then :)
|
Please squash. |
The JIT IR is designed to be small. There are two kinds of instruction: - short instructions, with inlined operands. - long ones (unimplemented as of yet) Ideally, function, block and instruction IDs (which are indices read from the on-disk AOT IR) would be either converted to references as we decode, but this would require changes to the decoder. We may have to ditch deku: sharksforarms/deku#383 Co-authored-by: Lukas Diekmann <[email protected]>
|
splat. |
|
|
Thanks! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Huh, we haven't touched any of the old JIT stuff so this test shouldn't fail. It may be related to the bug I'm working on at the moment. Though I failed to reproduce it locally even with |
|
I agree that this failure seems unrelated. Maybe kick it again and see if it merges? |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
again... |
|
Yep, so this isn't a transient error... |
|
It's probably this: #909 I can look into it in January. |
|
I just forced a build of this and it succeeded: So I think the failure is in fact transient, maybe it is #909. It's probably gonna need some try_repeat and thinking hard to fix. What shall we do with this PR? Retry? |
|
(Note that #909 predates this PR) |
|
Actually, I was working on a bug before Christmas that might fix this. I still have to make a test for the PR. Let's see if that works before looking into this further. |
|
Since this PR appears unrelated to the failure, should we retry it? |
|
Yeah, let's go for it. |
As requested here: ykjit#930 (comment)
As requested here: ykjit#930 (comment)
The JIT IR is designed to be small. There are two kinds of instruction:
Ideally, function, block and instruction IDs (which are indices read from the on-disk AOT IR) would be either converted to references as we decode, but this would require changes to the decoder. We may have to ditch deku:
sharksforarms/deku#383