Some random comment, only looked at Bytecode emitter so far.
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#newcode6
src/interpreter/bytecode-array-builder.cc:6: #include
"src/interpreter/bytecode-array-builder.h"
always have header on it's own line first (which means you will need to
add "src/ast.h" to the header) to ensure the header includes all the
includes it needs.
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode16
src/interpreter/bytecode-array-builder.cc:16: void
BytecodeArrayBuilder::set_stack_slots(int slots) {
nit - this would be clearer to me as this be set_locals_count?
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode17
src/interpreter/bytecode-array-builder.cc:17: scratch_register_ = slots;
local_register_count_
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode18
src/interpreter/bytecode-array-builder.cc:18: max_registers_ = slots;
max_temporary_register_count_
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode22
src/interpreter/bytecode-array-builder.cc:22: Handle<BytecodeArray>
BytecodeArrayBuilder::ToBytecodeArray() const {
add a "bytecodes_generated_" bool and DCHECK here that this is not set,
before setting it at the end of the function.
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode76
src/interpreter/bytecode-array-builder.cc:76: int
BytecodeArrayBuilder::AllocateScratchRegister() {
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).
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode77
src/interpreter/bytecode-array-builder.cc:77: int reg =
scratch_register_++;
nit - add CHECK(locals_register_count_ >= 0)
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode91
src/interpreter/bytecode-array-builder.cc:91: void
BytecodeArrayBuilder::Output(Bytecode bytecode, uint8_t r0, uint8_t r1,
nit - operand0, operand1, etc. (instead of r0, r1...)
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode123
src/interpreter/bytecode-array-builder.cc:123: Bytecode
BytecodeArrayBuilder::BytecodeFor(Token::Value op) {
Let's make this BytecodeForBinaryOp and remove the ASSIGN case (which
I'm not sure would every be called in the current code?)
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.h#newcode24
src/interpreter/bytecode-array-builder.h:24: void
BinaryOperation(Token::Value binop, int reg);
optional suggestion - is it worth returning "this" from each builder op
to allow builder style like:
builder().LoadLiteral()
.LoadUndefined()
.LoadFalse()
?
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.h#newcode25
src/interpreter/bytecode-array-builder.h:25: void
LoadLiteral(v8::internal::Smi* value);
nit - add some section header comments (e.g., "Load constants",
"register management", etc.).
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecodes.h#newcode27
src/interpreter/bytecodes.h:27: V(LoadSmi8, OperandType::kImm8) \
nit - can we update the bytecodes above to be in the same form as below
(i.e., Ldr0, LdrSmi8 and also add Lda0 and LdaSmi8?)
Also, could we split up the list with some heading comments (e.g.,
Constants, Register moves, Binary Ops, etc.). And maybe add comments for
some of the more difficult to parse operations (e.g, Ldar - "Loads
accumulator from a register.")
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/interpreter.cc#newcode54
src/interpreter/interpreter.cc:54: bytecodes->Print();
nit - add a flag for printing the bytecodes (e.g.,
FLAG_trace_ignition_bytecodes, and possibly also rename
FLAG_trace_ignition to FLAG_trace_ignition_handler_generation or
similar).
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/interpreter.cc#newcode58
src/interpreter/interpreter.cc:58: //
info->SetCode(info->isolate()->builtins()->InterpreterEntryTrampoline());
Just do this?
https://codereview.chromium.org/1266713004/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
