Skip to content

Conversation

@undingen
Copy link
Contributor

@undingen undingen commented Oct 5, 2016

transform BST to gapless bytecode
- all instructions of the CFG are copied into a single memory region.
- all instructions are packed and follow each other directly
- the CFGBlock just point into the bytecode block where the first instruction of the block is located

@undingen
Copy link
Contributor Author

undingen commented Oct 6, 2016

@kmod could you please do a initial review.
I think the first patches are fine but the last two (interned string change + the actual bytecode change) are little ugly.
The main problems for me are:

  • the InternedString could in most cases just be a BoxedString* (we also already intern all of them)
  • I don't like that I had to copy the CFG and CFGBlock classes and how I update the CFGBlock* inside the instructions. Probably I should remove the pointers and use indices instead.
  • The whole copying of nodes is ugly and we should definitely use reader/writer methods instead of directly using the struct layout (this is helps with implementing the "set one bit in the opcode for 1byte vregs instead of 4byte"
  • enforce that all blocks actual have a single terminator at the end because this is how we know when we reached the end of a CFGBlock

@kmod
Copy link
Collaborator

kmod commented Oct 7, 2016

At this point InternedString is just a thin wrapper around an interned BoxedString, so it should definitely be possible to get rid of it. I'm actually pretty ok with not doing that just yet though: we need to do a larger change to properly use the fact that CPython's parser returns all these objects as Python objects (rather than doing the "PyObject -> serialized version -> getFooConstant" dance we currently do). So anyway the way you have it seems totally fine to me.

I'm a little bit more concerned about the bytecode-packing change because it feels like it's a bit of a detour away from the way we probably want to do it in the long run (constructing the bytecode directly in-place). How much work do you think it'd be to do that? I don't have a huge opinion about either getting it in as-is or trying to take some more time to do the construct-in-place.

@undingen
Copy link
Contributor Author

undingen commented Oct 8, 2016

I split all changes except the bytecode packing one into a new PR.
I agree with your concerns and I worked on a new way of this patch which directly inplace allocates the instruction gapless. Unfortunately I did not get it ready before my holiday :-( (it's trickier then I thought)
I'm a little worried that with directly emitting the instructions the use 1byte vregs instead of 4 when possible is harder to implement. With this copying approach it should be quite easy to add.

- all instructions of the CFG are copied into a single memory region.
- all instructions are packed and follow each other directly
- the CFGBlock just point into the bytecode block where the first instruction of the block is located
@undingen
Copy link
Contributor Author

closing: this approach got replaced by the one in #1391

@undingen undingen closed this Nov 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants