Resolve br/br_if/br_table in parser #312
Conversation
gumb0
force-pushed
the
control-stack-parser
branch
3 times, most recently
from
c2a229c
to
6530093
Compare
Codecov Report
@@ Coverage Diff @@
## master #312 +/- ##
==========================================
+ Coverage 98.78% 98.84% +0.05%
==========================================
Files 38 38
Lines 11255 11468 +213
==========================================
+ Hits 11118 11335 +217
+ Misses 137 133 -4 |
gumb0
force-pushed
the
control-stack-parser
branch
10 times, most recently
from
4501516
to
c7e8d2c
Compare
gumb0
force-pushed
the
control-stack-parser
branch
4 times, most recently
from
b32a20c
to
6dc8696
Compare
gumb0
force-pushed
the
control-stack-parser
branch
from
6dc8696
to
9fa554f
Compare
gumb0
force-pushed
the
control-stack-parser
branch
2 times, most recently
from
7d78346
to
5cb457c
Compare
gumb0
force-pushed
the
control-stack-parser
branch
5 times, most recently
from
ca8ae8d
to
8d201ff
Compare
gumb0
force-pushed
the
control-stack-parser
branch
from
158b7b2
to
85ee738
Compare
lib/fizzy/execute.cpp
Outdated
| const auto imm_offset = read<uint32_t>(immediates); | ||
| // TODO: these will be const when blocks other than loop use them | ||
| size_t stack_height = static_cast<uint32_t>(read<int>(immediates)); | ||
| auto arity = read<uint8_t>(immediates); |
There was a problem hiding this comment.
Is the reading happening here for the label.pc == nullptr just to skip ahead? Would it make sense using skip/read in the branches below?
There was a problem hiding this comment.
Yes, but these branches are removed in later commits and then it's used always
gumb0
force-pushed
the
control-stack-parser
branch
from
85ee738
to
e048cd4
Compare
gumb0
force-pushed
the
control-stack-parser
branch
from
e048cd4
to
e39fb28
Compare
|
Rebased. |
axic
changed the title
| if (!labels.empty()) | ||
| labels.pop(); | ||
| if (labels.empty()) | ||
| if (pc == &code.instructions[code.instructions.size()]) |
There was a problem hiding this comment.
Should have a comment this is about aborting after the "final" end.
| push(immediates, static_cast<uint32_t>(frame.parent_stack_height)); | ||
| // Always pushing 0 as loop's arity, because br executed in loop jumps to the top, | ||
| // resetting frame stack to 0, so return type arity is not important for br resolution. | ||
| push(immediates, frame.instruction == Instr::loop ? uint8_t{0} : frame.arity); |
There was a problem hiding this comment.
If arity for loop is ignored, couldn't we just always pass frame.arity to avoid this extra branch? Likely it is an unneeded micro-optimization at this stage.
There was a problem hiding this comment.
No, if loop has a result, it still needs to clear all the stack when executing br, not keeping one element like br out of block or if.
This way it's less branching in execution - branching code doesn't care from what kind of block its jumping, and just takes destination and arity from immediates.
There was a problem hiding this comment.
Rephrased the comment slightly.
There was a problem hiding this comment.
This comment wording to me still suggests that loop always gets arity 0, just like the code does.
gumb0
force-pushed
the
control-stack-parser
branch
from
e39fb28
to
b3eed80
Compare
gumb0
force-pushed
the
control-stack-parser
branch
from
b3eed80
to
e8ff26f
Compare
Replaces #297
I tried to make the change history to gradually introduce resolving in parser, at first keeping control stack in execute and old instruction immediates, then deleting it in the very end. So this can be reviewed by commit.
For
br_tableit currently puts arity into immedaites for each label. This should be further optimized to have one arity immediate for entirebr_tableinstruction.