Skip to content

Conversation

@damnMeddlingKid
Copy link

@damnMeddlingKid damnMeddlingKid commented Feb 28, 2022

What

This PR implements a proof of concept for handling branches in the Liquid VM.

How

OP Codes

To implement branches i've added 3 OP codes:

  • OP_EVAL_CONDITION: This operation has a constant which is a Liquid Condition object. When executed it evaluates the Condition and pushes the RTEST of the result onto the stack
  • OP_BRANCH_UNLESS: Pops a value off the stack, if its not true then jump to the offset located in the lower 16bits of the instruction.
  • OP_BRANCH: This is an unconditional branch, it's used to exit out of a block to the "endif".

Process

When an if tag is encountered we add an OP_EVAL_CONDITION for the if statement and add an OP_BRANCH_UNLESS operation that jumps to the next clause (or endif)

Parsing then resumes as normal by calling internal_block_body_parse, when internal_block_body_parse returns with an unknown tag we check if its one of the clauses we can handle like else if so we add an unconditional OP_BRANCH which exits to the next endif and continue parsing the tokens for the else block.

Nesting is handled through recursion.

Heres an example of a liquid if block and its corresponding bytecode.

image

Theres a more complete example in test.rb with elsif and nesting.

Limitations of the POC

I'm sure theres a lot thats missing, heres the stuff im aware of:

  • Currently i've only parsed conditions with a single binary comparison, this needs to be extended to handle more comparisons
  • In order to support multiple elsif clauses i've added a small array that can handle 10 clauses, this needs to be dynamically sized.
  • Theres no validation of the structure of the tag. right now its possible to have an else before an elsif
  • No restriction on the nesting depth


uint8_t* instruction;

ptrdiff_t exit_branches[10];
Copy link
Author

@damnMeddlingKid damnMeddlingKid Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This array is used to keep track of all the branches that need to jump to "endif". This needs to use a dynamically sized list here, something like cbuffer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The offsets get stored with the instruction in 16 bits. assuming this is ok for a jump offset, otherwise need to store them somewhere else.

return unknown_tag;
}

VALUE parse_single_binary_comparison(VALUE markup) {
Copy link
Author

@damnMeddlingKid damnMeddlingKid Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only supports parsing 1 binary comparison, need to extend this to support more conditions.

*exit_end++ = vm_assembler_open_branch(body_code, OP_BRANCH);

// Calculate the offset that would jump to here, this is where the <if> jumps to if it fails the condition.
jump = (ptrdiff_t) (body_code->instructions.data_end - body_code->instructions.data);
Copy link
Author

@damnMeddlingKid damnMeddlingKid Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calculating indices since cbuffer can realloc.


// Resolve the open branch from the <if> with the calculated offset.
vm_assembler_close_branch(body_code, open_branch, jump);
open_branch = -1;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using -1 as a sentinel value to indicate no open branch.

render_score_increment += 1;
body->as.intermediate.blank = false;
break;
} else if (
Copy link
Author

@damnMeddlingKid damnMeddlingKid Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of scrappy, need a better abstraction for this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of something along the lines of #96 so we could pick the right parsing handler from ruby land.

Comment on lines +223 to +224
render_score_increment += 1;
body->as.intermediate.blank = false;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what these two do, might be updating them incorrectly.

@damnMeddlingKid damnMeddlingKid changed the title Franklyn/reviewable Branches Proof Of Concept Mar 1, 2022
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.

1 participant