Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1257543003:
[Interpreter] Add more bytecode definitions and add operand types. (Closed)
Description[Interpreter] Add more bytecode definitions and add operand types.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/4da6cbd9ee4f4546072ce878fd9ccd72f3fb532d
Cr-Commit-Position: refs/heads/master@{#29934}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Incorporate comments on patch set 1. #Patch Set 3 : Preprocessor robust method of describing bytecode operands. #
Total comments: 30
Patch Set 4 : Attempt to address patch set 3 comments. #
Total comments: 18
Patch Set 5 : Address comments on patch set 4. #Patch Set 6 : Nits. #Patch Set 7 : Rebase. #Patch Set 8 : Fix comment. #Patch Set 9 : Rebase. #
Messages
Total messages: 22 (6 generated)
oth@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL, thanks!
Not read through the whole thing yet, but a couple of comments / suggestions https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:5: #define OPERANDS(x) OperandInfo##x Is this required? https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:17: static const int is_explicit; instead of having is_explicit, could we just special-case None (and if we remove Imm0 and keep all the other operands to be byte size currently this would simplify things a lot I think - potentially getting rid of the need for the OperatorInfo struct). WDYT? https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:29: OPERAND_INFO(Imm0, 0); What is Imm0? Seems unrequired. https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:31: OPERAND_INFO(Smi, 4); As mentioned offline, this can't be a 4 byte operand - we should make it take an index into the constant pool (or we can remove it for now and just have Imm8 for now). I actually think we should have all operands be just 1 byte for now for simplicity until we need something with 16bits or larger range - WDYT? https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:51: PACK_OPERAND(op0, 0) | PACK_OPERAND(op1, 1) | PACK_OPERAND(op2, 2) I personally don't think it's worth packing operands into a single integer - we are going to end up with a bytecode which requires more than 4 operands at some point and then this will no longer work. It's difficult to work out how you are using this without the generator CL, but would it work if we had these as a std::vector in BytecodeInfo, and create a constant array of BytecodeInfo which can be accessed by index (to avoid having to allocate BytecodeInfo's all over the place)? https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:55: #define BYTECODE_INFO(code, op0, op1, op2) \ As much as I like pre-processor magic </sarcasm> - does this need to be done as a macro? Couldn't it just be a class or struct which takes the bytecodes and operands as constructor arguments, or passing them as template arguments? https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:76: BYTECODE_INFO(kReturn, Operand::kNone, Operand::kNone, Operand::kNone); I would really like this list of declarations to be in the same place as BYTECODE_LIST if possible. Is there any way we could get closer to what you had when you showed me it the other day (i.e., "V(Move, Operands(Reg, Reg,Reg))" )? https://codereview.chromium.org/1257543003/diff/1/src/interpreter/interpreter.cc File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1257543003/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.cc:58: UNIMPLEMENTED(); This will fail when we build the interpreter-table (which we won't want otherwise we can't test anything). We should figure out a way of generating code which causes an abort when run (not sure how we would do that in TF, but I'll have a look).
On 2015/07/24 18:34:48, rmcilroy wrote: > Not read through the whole thing yet, but a couple of comments / suggestions > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc > File src/interpreter/bytecodes.cc (right): > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:5: #define OPERANDS(x) OperandInfo##x > Is this required? > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:17: static const int is_explicit; > instead of having is_explicit, could we just special-case None (and if we remove > Imm0 and keep all the other operands to be byte size currently this would > simplify things a lot I think - potentially getting rid of the need for the > OperatorInfo struct). WDYT? > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:29: OPERAND_INFO(Imm0, 0); > What is Imm0? Seems unrequired. > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:31: OPERAND_INFO(Smi, 4); > As mentioned offline, this can't be a 4 byte operand - we should make it take an > index into the constant pool (or we can remove it for now and just have Imm8 for > now). > > I actually think we should have all operands be just 1 byte for now for > simplicity until we need something with 16bits or larger range - WDYT? > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:51: PACK_OPERAND(op0, 0) | PACK_OPERAND(op1, 1) | > PACK_OPERAND(op2, 2) > I personally don't think it's worth packing operands into a single integer - we > are going to end up with a bytecode which requires more than 4 operands at some > point and then this will no longer work. > > It's difficult to work out how you are using this without the generator CL, but > would it work if we had these as a std::vector in BytecodeInfo, and create a > constant array of BytecodeInfo which can be accessed by index (to avoid having > to allocate BytecodeInfo's all over the place)? > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:55: #define BYTECODE_INFO(code, op0, op1, op2) > \ > As much as I like pre-processor magic </sarcasm> - does this need to be done as > a macro? Couldn't it just be a class or struct which takes the bytecodes and > operands as constructor arguments, or passing them as template arguments? > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.c... > src/interpreter/bytecodes.cc:76: BYTECODE_INFO(kReturn, Operand::kNone, > Operand::kNone, Operand::kNone); > I would really like this list of declarations to be in the same place as > BYTECODE_LIST if possible. Is there any way we could get closer to what you had > when you showed me it the other day (i.e., "V(Move, Operands(Reg, Reg,Reg))" )? > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/interpreter.cc > File src/interpreter/interpreter.cc (right): > > https://codereview.chromium.org/1257543003/diff/1/src/interpreter/interpreter... > src/interpreter/interpreter.cc:58: UNIMPLEMENTED(); > This will fail when we build the interpreter-table (which we won't want > otherwise we can't test anything). We should figure out a way of generating code > which causes an abort when run (not sure how we would do that in TF, but I'll > have a look). Thanks! Patch set 2 is a simpler/cleaner implementation based on comments.
+danno
This looks much better to me. A couple of comments, most of which are nits. https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interprete... File src/compiler/interpreter-assembler.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interprete... src/compiler/interpreter-assembler.cc:92: Node* InterpreterAssembler::BytecodeArg(int delta) { Could you change this to BytecodeOperand as well please. https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:19: { __VA_ARGS__ } \ Does C++ always zero-initialize entries in an array? i.e., does: { Operand::kImm8 } turn into { 1, 0, 0 } here implicitly, or are entries [1] and [2] undefined? https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:49: CHECK(bytecode <= Bytecode::kLast); DCHECK https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:56: CHECK(bytecode <= Bytecode::kLast); DCHECK https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:70: CHECK(bytecode <= Bytecode::kLast && i < NumberOfOperands(bytecode)); DCHECK https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:19: #define OPERAND_LIST(V) \ nit - OPERAND_TYPE_LIST https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:24: enum class Operand : uint8_t { nit - OperandType https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:39: V(Return, Operand::kNone) nit - can we move this up just below OPERAND_LIST just to make these declarations obvious. https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:62: // Returns bytecode for |value|,, checking validity in the process. remove extra ',' https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:68: // Return the i-th operand of |bytecode|. Return the type of the i-th operand of |bytecode| https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:69: static const Operand GetOperand(Bytecode bytecode, int i); GetOperandType https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:49: void Interpreter::DoLoadSmi0(compiler::InterpreterAssembler* assembler) { I think we should keep this as LoadLiteral0 so that it is distinct from LoadSmi8 (which doesn't actually load '8' but loads 8bits. https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:56: // LoadSmi8 dst, imm8 nit - could you update all the comments to have this form and add a small commentary below it like the other comments. https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:57: void Interpreter::DoLoadSmi8(compiler::InterpreterAssembler* assembler) { Is LoadLiteralSmi8 too wordy? (honest question).
That's for the last round of comments, updated the patch. https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interprete... File src/compiler/interpreter-assembler.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interprete... 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/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... 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/interpr... 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/interpr... 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?
Missing comment. https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:19: { __VA_ARGS__ } \ On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote: > Does C++ always zero-initialize entries in an array? i.e., does: > { Operand::kImm8 } > turn into > { 1, 0, 0 } > here implicitly, or are entries [1] and [2] undefined? Yes, C++ will initialize these to zero provided at least one value is given. For readability the declarations could include kNone as the last operand type or use templates to pad out the arguments to explicitly include 0's. The current nit that grates here is counting the operands at runtime rather than compile time. The current pattern is more readable, but would be better to determine at compile time.
lgtm once nits are addressed. https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:19: { __VA_ARGS__ } \ On 2015/07/27 17:09:53, oth wrote: > On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote: > > Does C++ always zero-initialize entries in an array? i.e., does: > > { Operand::kImm8 } > > turn into > > { 1, 0, 0 } > > here implicitly, or are entries [1] and [2] undefined? > > Yes, C++ will initialize these to zero provided at least one value is given. For > readability the declarations could include kNone as the last operand type or use > templates to pad out the arguments to explicitly include 0's. OK sounds good. > The current nit that grates here is counting the operands at runtime rather than > compile time. The current pattern is more readable, but would be better to > determine at compile time. I'm not too worried about this since it will only be run when building the interpreter bytecode handlers (usually at snapshot generation time), but if you are worried you could build a separate table of operand counts using a hacky trick like: #define NUM_VAR_ARGS(...) (sizeof((uint8_t[]){__VA_ARGS__}) https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:57: void Interpreter::DoLoadSmi8(compiler::InterpreterAssembler* assembler) { On 2015/07/27 16:59:00, oth wrote: > 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? No, I'd rather have the handlers named the same as the bytecode in disassembly. This is fine I think. https://codereview.chromium.org/1257543003/diff/60001/src/compiler/interprete... File src/compiler/interpreter-assembler.h (right): https://codereview.chromium.org/1257543003/diff/60001/src/compiler/interprete... src/compiler/interpreter-assembler.h:44: // Returns the bytecode argument |index| for the current bytecode. update comment too please. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:20: , nit - comma on the line above? https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:18: // The list of operands used by bytecodes. /s/operands/operand types/ https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:29: nit - extra newline https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:41: nit - extra newline https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:63: // Returns bytecode for |value|, checking validity in the process. nit - remove the ", checking validity in the process" since this is now a DCHECK. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.cc:50: // argument. /s/the register index specified by the bytecode's argument/the destination register https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.cc:62: UNIMPLEMENTED(); nit - just make this //TODO(rmcilroy) Implement LoadSmi8 for now. https://codereview.chromium.org/1257543003/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1257543003/diff/60001/src/objects.cc#newcode1... src/objects.cc:11637: os << bytecode << "\n"; could we add the formatted versions of the arguments after the bytecode - e.g., r1 for register 1 and #123 for literal SMI 123, etc? (fine to do in another CL).
Thanks, done a couple of exceptions (git cl format, work in other patch). https://codereview.chromium.org/1257543003/diff/60001/src/compiler/interprete... File src/compiler/interpreter-assembler.h (right): https://codereview.chromium.org/1257543003/diff/60001/src/compiler/interprete... src/compiler/interpreter-assembler.h:44: // Returns the bytecode argument |index| for the current bytecode. On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > update comment too please. Done. sed'ited it and missed this, doh! https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:20: , On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > nit - comma on the line above? git cl format, leaving for now to avoid the presubmit warning. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:18: // The list of operands used by bytecodes. On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > /s/operands/operand types/ Done. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:29: On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > nit - extra newline Done. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:41: On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > nit - extra newline Done. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecod... src/interpreter/bytecodes.h:63: // Returns bytecode for |value|, checking validity in the process. On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > nit - remove the ", checking validity in the process" since this is now a > DCHECK. Done. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.cc:50: // argument. On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > /s/the register index specified by the bytecode's argument/the destination > register Done. https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.cc:62: UNIMPLEMENTED(); On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > nit - just make this //TODO(rmcilroy) Implement LoadSmi8 for now. Done. https://codereview.chromium.org/1257543003/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1257543003/diff/60001/src/objects.cc#newcode1... src/objects.cc:11637: os << bytecode << "\n"; On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > could we add the formatted versions of the arguments after the bytecode - e.g., > r1 for register 1 and #123 for literal SMI 123, etc? (fine to do in another CL). Part of next patch.
On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... > src/interpreter/interpreter.cc:57: void > Interpreter::DoLoadSmi8(compiler::InterpreterAssembler* assembler) { > On 2015/07/27 16:59:00, oth wrote: > > 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? > > No, I'd rather have the handlers named the same as the bytecode in disassembly. > This is fine I think. The names are particularly important today, but LoadLiteralSmi8 will be insufficient now we're moving to registers plus accumulator assuming the interpreter supports loads to both. (LdaSmi8, LdrSmi8).
On 2015/07/28 13:35:28, oth wrote: > On 2015/07/28 08:58:12, rmcilroy (OOO until 10th Aug) wrote: > > > https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpr... > > src/interpreter/interpreter.cc:57: void > > Interpreter::DoLoadSmi8(compiler::InterpreterAssembler* assembler) { > > On 2015/07/27 16:59:00, oth wrote: > > > 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? > > > > No, I'd rather have the handlers named the same as the bytecode in > disassembly. > > This is fine I think. > > The names are particularly important today, but LoadLiteralSmi8 will be > insufficient now we're moving to registers plus accumulator assuming the > interpreter supports loads to both. > (LdaSmi8, LdrSmi8). s/important/not important/ :-)
The CQ bit was checked by oth@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1257543003/#ps140001 (title: "Fix comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1257543003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1257543003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/compiler/interpreter-assembler.cc:
While running git apply --index -3 -p1;
error: patch failed: src/compiler/interpreter-assembler.cc:89
Falling back to three-way merge...
Applied patch to 'src/compiler/interpreter-assembler.cc' with conflicts.
U src/compiler/interpreter-assembler.cc
Patch: src/compiler/interpreter-assembler.cc
Index: src/compiler/interpreter-assembler.cc
diff --git a/src/compiler/interpreter-assembler.cc
b/src/compiler/interpreter-assembler.cc
index
691fa61db576a0b7030373df7e51da2b405b6ce9..45466af8c0c4f10b4985719ac3db52664608895b
100644
--- a/src/compiler/interpreter-assembler.cc
+++ b/src/compiler/interpreter-assembler.cc
@@ -89,8 +89,8 @@ Node* InterpreterAssembler::RegisterFrameOffset(Node* index) {
}
-Node* InterpreterAssembler::BytecodeArg(int delta) {
- DCHECK_LT(delta, interpreter::Bytecodes::NumberOfArguments(bytecode_));
+Node* InterpreterAssembler::BytecodeOperand(int delta) {
+ DCHECK_LT(delta, interpreter::Bytecodes::NumberOfOperands(bytecode_));
return raw_assembler_->Load(kMachUint8, BytecodePointer(),
Int32Constant(1 + delta));
}
The CQ bit was checked by oth@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1257543003/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1257543003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1257543003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4da6cbd9ee4f4546072ce878fd9ccd72f3fb532d Cr-Commit-Position: refs/heads/master@{#29934} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
