That's for the last round of comments, updated the patch.
https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interpreter-assembler.cc
File src/compiler/interpreter-assembler.cc (right):
https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interpreter-assembler.cc#newcode92
src/compiler/interpreter-assembler.cc:92: Node*
InterpreterAssembler::BytecodeArg(int delta) {
On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote:
Could you change this to BytecodeOperand as well please.
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc#newcode49
src/interpreter/bytecodes.cc:49: CHECK(bytecode <= Bytecode::kLast);
On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote:
DCHECK
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc#newcode56
src/interpreter/bytecodes.cc:56: CHECK(bytecode <= Bytecode::kLast);
On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote:
DCHECK
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc#newcode70
src/interpreter/bytecodes.cc:70: CHECK(bytecode <= Bytecode::kLast && i
< NumberOfOperands(bytecode));
On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote:
DCHECK
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode19
src/interpreter/bytecodes.h:19: #define OPERAND_LIST(V) \
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
nit - OPERAND_TYPE_LIST
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode24
src/interpreter/bytecodes.h:24: enum class Operand : uint8_t {
On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote:
nit - OperandType
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode39
src/interpreter/bytecodes.h:39: V(Return, Operand::kNone)
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
nit - can we move this up just below OPERAND_LIST just to make these
declarations obvious.
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode62
src/interpreter/bytecodes.h:62: // Returns bytecode for |value|,,
checking validity in the process.
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
remove extra ','
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode68
src/interpreter/bytecodes.h:68: // Return the i-th operand of
|bytecode|.
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
Return the type of the i-th operand of |bytecode|
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode69
src/interpreter/bytecodes.h:69: static const Operand GetOperand(Bytecode
bytecode, int i);
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
GetOperandType
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc#newcode49
src/interpreter/interpreter.cc:49: void
Interpreter::DoLoadSmi0(compiler::InterpreterAssembler* assembler) {
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
I think we should keep this as LoadLiteral0 so that it is distinct
from LoadSmi8
(which doesn't actually load '8' but loads 8bits.
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc#newcode56
src/interpreter/interpreter.cc:56: // LoadSmi8 dst, imm8
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
nit - could you update all the comments to have this form and add a
small
commentary below it like the other comments.
Done.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc#newcode57
src/interpreter/interpreter.cc:57: void
Interpreter::DoLoadSmi8(compiler::InterpreterAssembler* assembler) {
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
Is LoadLiteralSmi8 too wordy? (honest question).
There are two issues here - how handlers are named and what the
disassembly looks like. Personally, I'd lean towards compact and easy to
scan disassembler output. Handlers could have the more descriptive names
and the disassembly a compact and easy scan format by adding an extra
argument to DECLARE_BYTECODE?
https://codereview.chromium.org/1257543003/
--
--
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.
