Also:
- Update title to mention this also adds a BytecodeArrayBuilder
- Update description to have the same first line as title (this is what
actually get's used in the commit log).
- Add a bit more detail in the description
- Would be nice to have some simple unittests for the
BytecodeArrayBuilder (in
test/unittest/interpreter)
Other than that, looks great - lgtm once the comments and above are done.
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc
File src/interpreter/bytecode-array-builder.cc (right):
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode76
src/interpreter/bytecode-array-builder.cc:76: int
BytecodeArrayBuilder::AllocateScratchRegister() {
On 2015/07/30 15:38:42, oth wrote:
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
> These should be Pop/PushScratchRegister if they require that the
scratches are
> pushed and popped in order. Also could we call them TempRegister
instead
> (scratch is typically something which is only live for a very short
time,
where
> these might be live for the whole of an expression).
Done.
Missed rename to Push/Pop?
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.cc
File src/interpreter/bytecode-array-builder.cc (right):
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.cc#newcode29
src/interpreter/bytecode-array-builder.cc:29: int frame_size =
register_count * sizeof(intptr_t);
replace sizeof(intptr_t) with kPointerSize (otherwise when cross
compiling the framesize will be wrong in the serialized snapshot).
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.h#newcode25
src/interpreter/bytecode-array-builder.h:25: // Set number of locals
required for bytecode.
s/for bytecode/by bytecode array/
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.h#newcode36
src/interpreter/bytecode-array-builder.h:36: //
===========================================================================
nit - this type of header format isn't very common in V8. I would just
have:
// Constant loads to accumulator
BytecodeArrayBuilder& LoadLiteral....
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecodes.h#newcode28
src/interpreter/bytecodes.h:28: \
super nit - remove newline below comment.
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode51
src/interpreter/interpreter.cc:51: // LdaZero <dst>
Remove <dst>
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode53
src/interpreter/interpreter.cc:53: // Load literal '0' into the
destination register.
update comment /s/destination/accumulator/ and remove the code
generation (since it's wrong now).
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode61
src/interpreter/interpreter.cc:61: // LdaSmi8 <dst>, <imm8>
remove <dst>
https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode63
src/interpreter/interpreter.cc:63: // Load an 8-bit integer literal into
destination register as a Smi.
/s/destination/acumulator
https://codereview.chromium.org/1266713004/diff/40001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1266713004/diff/40001/src/objects.cc#newcode11633
src/objects.cc:11633: case interpreter::OperandType::kNone:
We should never hit this case, right? If so, maybe just add
UNREACHABLE() here (and move to the bottom)
https://codereview.chromium.org/1266713004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
