Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InstIdx as a u16 is probably too small: move to U24 #1238

Open
ltratt opened this issue Jun 11, 2024 · 1 comment
Open

InstIdx as a u16 is probably too small: move to U24 #1238

ltratt opened this issue Jun 11, 2024 · 1 comment
Assignees

Comments

@ltratt
Copy link
Contributor

ltratt commented Jun 11, 2024

InstIdx is nominally 16 bits big, but PackedOperand -- which is often used to represent InstIdxs -- only has 15 bits to represent InstIdx. That means trace_builder can only create a trace of 32767 instructions before we literally have to give up. We are likely to find traces in at least the low thousands of instructions (before optimisation) to be worth compiling, and we could easily find that longer traces are useful. I can easily imagine 10,000 instruction long traces being useful for some workloads, and maybe more beyond that, especially since AOT code will never be fully optimised. 32767 feels to me like it may well be a little too low a number for comfort.

My suggestion is that we do the following make InstIdx and PackedOperand use U24. That allows us to represent 8388607 instructions in a PackedOperand. [Honestly a U20 would probably be suffifcient, but non-byte-sized integers are probably more effort than they're worth.]

However, this will increase the size of Inst to 14 bytes, which is an annoying number (and that might even up being rounded up to 16 or 18 bytes after alignment). The reason for this is SelectInst which contains 3 PackedOperands: if this didn't exist we'd squeeze everything into 12 bytes. So I'd then make a special Vec in Module to hold just SelectInsts, and make SelectInst a wrapper around a u32 (or whatever) into that. It's not ideal, but since SelectInsts are probably not too numerous, this level of indirection is probably worth it.

@ltratt
Copy link
Contributor Author

ltratt commented Jul 1, 2024

I have a variant idea from this (influenced by LuaJIT): rather than having side-vectors, LuaJIT stores "bigger than a machine word instruction stuff" inline. For example, call arguments are special instructions that come after a CALL (searco for CARG in https://archive.is/RqAY7#selection-3473.105-3493.3). So, in this case, SelectInst might have a SelectInstBlah instruction which stores the extra stuff, allowing most instructions to still be a machine word.

This isn't a trivial change -- notably it would means that "iterate over all instruction indexes" would need to be changed -- but it's probably worth considering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants