Skip to content

Refactor: One bit type node tag#26

Closed
JuliaPoo wants to merge 1 commit intopylbbvfrom
refactor/one-bit-type-node-tag
Closed

Refactor: One bit type node tag#26
JuliaPoo wants to merge 1 commit intopylbbvfrom
refactor/one-bit-type-node-tag

Conversation

@JuliaPoo
Copy link
Copy Markdown

@JuliaPoo JuliaPoo commented Jun 8, 2023

No description provided.

@JuliaPoo JuliaPoo requested a review from Fidget-Spinner June 8, 2023 14:54
@Fidget-Spinner
Copy link
Copy Markdown

I'm against this change because TYPE_NULL does help us debug better. I've caught a lot of issues in the early days of the codegen when we accidentally introduced a raw pointer into the type prop.

@JuliaPoo
Copy link
Copy Markdown
Author

JuliaPoo commented Jun 8, 2023

Alright, then I'll keep it, and the negative type tag discussed in #4 will be the third bit. Can we guarantee that pointers are 8-bytes aligned? I.e., the first 3 bits are zeroed.

If we can't we can just enforce alignment of the type context but I'm not sure how difficult to maintain that will be (will msvc enforce this?).
Otherwise, I'm pretty sure we can think of a different format that doesn't rely on tagged pointers

@Fidget-Spinner
Copy link
Copy Markdown

Can we guarantee that pointers are 8-bytes aligned? I.e., the first 3 bits are zeroed.

If we want to support 32-bit systems, with 4-bytes alignment, that means the last 2 bits are zeroed at least on x86. We can constrain this project to x86.

There's still one more value left for negative type tag - the value 0b11 or 3.

@JuliaPoo
Copy link
Copy Markdown
Author

JuliaPoo commented Jun 8, 2023

Hmm i guess I can do that but that's a little messy bcuz now TYPE_ROOT can have two different tags. But that should work.
I'll split this into TYPE_ROOT_POSITIVE and TYPE_ROOT_NEGATIVE.

I'll experiment on this approach and see how it goes, and if it works I'll close this pull.

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

Successfully merging this pull request may close these issues.

2 participants