Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1266713004:
[Intepreter] Addition of BytecodeArrayBuilder and accumulator based bytecodes. (Closed)
Description[Intepreter] BytecodeArrayBuilder and accumulator based bytecodes.
The BytecodeArrayBuilder has responsibility for emitting the BytecodeArray. It will be used by the AST walker.
Bytecode now uses an accumulator plus registers rather being pure register based.
Update BytecodeArray::Disassemble to print operand information.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/6ab1f70e12d5a8d44a5e3df3960899f9265558a2
Cr-Commit-Position: refs/heads/master@{#29970}
Patch Set 1 #Patch Set 2 : Tweak BytecodeArray::Disassemble(). #
Total comments: 30
Patch Set 3 : Move generator to a separate CL and patch set 1 comments. #
Total comments: 18
Patch Set 4 : Incorporate patch set 3 comments. #Patch Set 5 : Add test for BytecodeArrayBuilder, fix initialization bug, and change temporary register api to sco… #Patch Set 6 : Rebase and fix FrameSizesLookGood. #Patch Set 7 : Fix MSVC/gcc-pedantic compilation. #
Total comments: 8
Messages
Total messages: 30 (12 generated)
oth@chromium.org changed reviewers: + rmcilroy@chromium.org
Ross, just sharing progress. Needs tests. Feedback appreciated. Thanks.
Some random comment, only looked at Bytecode emitter so far. https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... src/interpreter/bytecode-array-builder.cc:17: scratch_register_ = slots; local_register_count_ https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:18: max_registers_ = slots; max_temporary_register_count_ https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/interpr... 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/interpr... src/interpreter/interpreter.cc:58: // info->SetCode(info->isolate()->builtins()->InterpreterEntryTrampoline()); Just do this?
Thanks, good feedback as usual. https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... 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/bytecod... File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/bytecod... 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/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/interpr... 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/interpr... 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.
The CQ bit was checked by oth@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266713004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266713004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
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/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecod... 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/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... 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/bytecod... File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... 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/bytecod... 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/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:28: \ super nit - remove newline below comment. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:51: // LdaZero <dst> Remove <dst> https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... 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/interpr... src/interpreter/interpreter.cc:61: // LdaSmi8 <dst>, <imm8> remove <dst> https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... 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#newcode1... 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)
Thanks! All incorporated. Tests to follow on this CL before committing. I should have updated the comment on Push/Pop for temporary registers. The change could be confusing when looking at code. Semantics are more like Allocate/Free. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:29: int frame_size = register_count * sizeof(intptr_t); On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > replace sizeof(intptr_t) with kPointerSize (otherwise when cross compiling the > framesize will be wrong in the serialized snapshot). Done. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.h:25: // Set number of locals required for bytecode. On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > s/for bytecode/by bytecode array/ Done. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.h:36: // =========================================================================== On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > nit - this type of header format isn't very common in V8. I would just have: > // Constant loads to accumulator > BytecodeArrayBuilder& LoadLiteral.... Done. It's copied compiler/code-generator.h. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecod... src/interpreter/bytecodes.h:28: \ On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > super nit - remove newline below comment. Done. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:51: // LdaZero <dst> On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > Remove <dst> Done. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:53: // Load literal '0' into the destination register. On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > update comment /s/destination/accumulator/ and remove the code generation (since > it's wrong now). Done. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:61: // LdaSmi8 <dst>, <imm8> On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > remove <dst> Done. https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:63: // Load an 8-bit integer literal into destination register as a Smi. On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > /s/destination/acumulator Done. 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#newcode1... src/objects.cc:11633: case interpreter::OperandType::kNone: On 2015/07/31 09:56:19, rmcilroy (OOO until 10th Aug) wrote: > We should never hit this case, right? If so, maybe just add UNREACHABLE() here > (and move to the bottom) Done.
The CQ bit was checked by oth@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266713004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266713004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
The CQ bit was checked by oth@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266713004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266713004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
The CQ bit was checked by oth@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266713004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266713004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1266713004/#ps120001 (title: "Fix MSVC/gcc-pedantic compilation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266713004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266713004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6ab1f70e12d5a8d44a5e3df3960899f9265558a2 Cr-Commit-Position: refs/heads/master@{#29970}
Message was sent while issue was closed.
picksi@google.com changed reviewers: + picksi@google.com
Message was sent while issue was closed.
I missed the boat a bit a bit (as the code has landed!)... but I had a number of comments for next time someone touches the code! https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/byteco... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/byteco... src/interpreter/bytecode-array-builder.cc:147: DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), 3); This function does a DCHECK_EQ, the following functions use DCHECK, is this intentional? https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/byteco... src/interpreter/bytecode-array-builder.cc:196: return static_cast<Bytecode>(-1); Should we have an error ByteCode that is -1, instead of casting it here? https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc#newcode... src/objects.cc:11614: int bytes = 0; Can we name 'bytes' to say what this is (the size of a bytecode+params, the size of a function?)? It is not clear! https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc#newcode... src/objects.cc:11628: for (int j = 1; j < bytes; j++) { Should this for-loop be pulled out into a separate function? It looks to be conceptually at a lower level (disassembling a single instruction?). Also, the value of 'j' is used as j-1, j and j+1. Is there a way to rework this to not need some many variations!
Message was sent while issue was closed.
Thanks, have put changes into: https://codereview.chromium.org/1259193004 https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/byteco... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/byteco... src/interpreter/bytecode-array-builder.cc:147: DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), 3); On 2015/08/03 11:06:01, picksi wrote: > This function does a DCHECK_EQ, the following functions use DCHECK, is this > intentional? Fixed, thanks! https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/byteco... src/interpreter/bytecode-array-builder.cc:196: return static_cast<Bytecode>(-1); On 2015/08/03 11:06:01, picksi wrote: > Should we have an error ByteCode that is -1, instead of casting it here? In this instance, it's not recoverable error. The failure mode of UNIMPLEMENTED() is a fatal which makes the source easy to find. There are more operands to come here. The test for the BytecodeArrayBuilder checks all bytecodes can be generated by the builder. A function in the builder generating an invalid bytecode would probably get used. https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc#newcode... src/objects.cc:11614: int bytes = 0; On 2015/08/03 11:06:01, picksi wrote: > Can we name 'bytes' to say what this is (the size of a bytecode+params, the size > of a function?)? It is not clear! Reworked in https://codereview.chromium.org/1259193004 https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc#newcode... src/objects.cc:11628: for (int j = 1; j < bytes; j++) { On 2015/08/03 11:06:01, picksi wrote: > Should this for-loop be pulled out into a separate function? It looks to be > conceptually at a lower level (disassembling a single instruction?). > > Also, the value of 'j' is used as j-1, j and j+1. Is there a way to rework this > to not need some many variations! Reworked in https://codereview.chromium.org/1259193004. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
