Allow longer traces #1569
Allow longer traces #1569
Conversation
| @@ -3143,7 +3201,7 @@ mod tests { | |||
| /// Ensure that any given instruction fits in 64-bits. | |||
| #[test] | |||
| fn inst_size() { | |||
| assert!(mem::size_of::<Inst>() <= mem::size_of::<u64>()); | |||
There was a problem hiding this comment.
Docstring above needs updating.
There was a problem hiding this comment.
Me may as well say our instructions are u128 sized rather than "two 64-bit sized"
There was a problem hiding this comment.
Fixed in the force push.
|
I don't object to this because we do indeed reject a lot of traces. |
|
Works for me too. |
ltratt
force-pushed
the
allow_longer_traces
branch
from
efad158 to
278a83a
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
ltratt
force-pushed
the
allow_longer_traces
branch
from
278a83a to
a48307a
Compare
|
With luck the force push fixes the test that breaks in swt. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
OK so it seems after some local testing that the only way to get this to explode is to make swt take a very long time. I am inclined to |
|
Ah, hang on, it's ignored on swt, so my measuring that was pointless 🤦♂️ Let me see if I can find a "big enough" value that's not too slow... |
Although `InstIdx` looks to be 16 bits big, `PackedOperand` forces it to be 15 bits and that just isn't enough (32,767) for some of the traces we now see, particularly after peeling. Probably 19 bits would be big enough, but right now that level of bit fiddling isn't worth our effort. Instead, this commit brute forces `InstIdx` to 32 bits. That also forces `ConstIdx` to be 32 bits, to keep code churn to a minimum.
ltratt
force-pushed
the
allow_longer_traces
branch
from
a48307a to
928a06e
Compare
|
I finally worked out what I was doing wrong: the |
This PR is as much a RFC as "let's merge this". Right now, we reject a lot of traces as being "too long". There are two current causes for this:
It's easy to think these two cases are the same, but they're not. With
YK_LOG=3the first case will be detected as stop-tracing time; the second case at compilation time.This PR only tackles the second case: it allows yk to compile longer traces. First, I think we should give up on the 16/15-bit
InstIdxs, at least for now: it's a very small type, and probably too restrictive. 04d3e8a changes this to 32/31 bits. Note:Insts now become two machine words big. I think this is fine for now.That then allows us to increase the size of traces we'll compile. How high is too high? We don't yet know, but it seems worth it to me to substantially increase the size 20,000 is still, I suspect, a fairly conservative limit efad158. But we're going to have to experiment with longer traces in order to know "how long is too long", so perhaps we should go higher, at least temporarily.