Thanks, good feedback as usual.
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"
On 2015/07/30 10:12:25, rmcilroy (OOO until 10th Aug) wrote:
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.
Done.
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) {
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
nit - this would be clearer to me as this be set_locals_count?
Done.
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode17
src/interpreter/bytecode-array-builder.cc:17: scratch_register_ = slots;
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
local_register_count_
Done.
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode18
src/interpreter/bytecode-array-builder.cc:18: max_registers_ = slots;
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
max_temporary_register_count_
Done.
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 {
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
add a "bytecodes_generated_" bool and DCHECK here that this is not
set, before
setting it at the end of the function.
Done.
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 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.
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_++;
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
nit - add CHECK(locals_register_count_ >= 0)
Done.
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,
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
nit - operand0, operand1, etc. (instead of r0, r1...)
Done.
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) {
On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
Let's make this BytecodeForBinaryOp and remove the ASSIGN case (which
I'm not
sure would every be called in the current code?)
Done.
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);
On 2015/07/30 10:12:25, rmcilroy (OOO until 10th Aug) wrote:
optional suggestion - is it worth returning "this" from each builder
op to allow
builder style like:
builder().LoadLiteral()
.LoadUndefined()
.LoadFalse()
?
Done.
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);
On 2015/07/30 10:12:25, rmcilroy (OOO until 10th Aug) wrote:
nit - add some section header comments (e.g., "Load constants",
"register
management", etc.).
Done.
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) \
On 2015/07/30 10:12:25, rmcilroy (OOO until 10th Aug) wrote:
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.")
Done.
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecodes.h#newcode27
src/interpreter/bytecodes.h:27: V(LoadSmi8, OperandType::kImm8) \
On 2015/07/30 10:12:25, rmcilroy (OOO until 10th Aug) wrote:
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.")
The pattern has been to add instructions as needed. Constant loads are
to the accumulator at the moment. Probably better to tweak instructions
in the generator patch.
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();
On 2015/07/30 10:12:25, rmcilroy (OOO until 10th Aug) wrote:
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).
Done in the bytecode generation CL.
https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/interpreter.cc#newcode58
src/interpreter/interpreter.cc:58: //
info->SetCode(info->isolate()->builtins()->InterpreterEntryTrampoline());
On 2015/07/30 10:12:25, rmcilroy (OOO until 10th Aug) wrote:
Just do this?
Other CL.
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.
