Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2142273003:
[interpreter] Inline Star on dispatch for some bytecodes (Closed)
Description[interpreter] Inline Star on dispatch for some bytecodes
For some bytecodes it is beneficial to always look for a Star
bytecode when dispatching to the next and inline perform it
without dispatching to the Star handler.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/5f603e838a731ed5e3585d94c5b0889ab749f24b
Cr-Commit-Position: refs/heads/master@{#37904}
Patch Set 1 #Patch Set 2 : refactor and enable Star lookahead for more #Patch Set 3 : cleanup #Patch Set 4 : also enable on common Star dispatcher bytecodes from websites and Speedometer #Patch Set 5 : refactor and only enable for a conservative set of Bytecodes #Patch Set 6 : ran git cl format #
Total comments: 6
Patch Set 7 : fixed nitpicks #
Total comments: 25
Patch Set 8 : fixed test case, refactored a bit more #Patch Set 9 : lookahead on more bytecodes, they dispatch to Star nearly always #Patch Set 10 : FIXED: lookahead on more bytecodes (dispatch to Star nearly always) #
Total comments: 2
Patch Set 11 : remove 100% dispatching bytecodes again #
Total comments: 24
Patch Set 12 : refactor Advance, other comments #Patch Set 13 : move kInterpreterTraceBytecodeExit to the right place #
Total comments: 14
Patch Set 14 : remove unnecessary Binds, worked in last comments #
Total comments: 6
Patch Set 15 : skip InterpreterAssemblerTest.Jump for IsStarLookahead() bytecodes, comments #Patch Set 16 : remove and inline DispatchTo to the single call site #
Total comments: 4
Patch Set 17 : restrict InterpreterAssemblerTest.Jump to jumps #
Messages
Total messages: 43 (23 generated)
The CQ bit was checked by klaasb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 klaasb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [interpreter] Look ahead for Star after Add/Sub/Mul BUG=v8:4280 LOG=N ========== to ========== [interpreter] Inline Star on dispatch for some bytecodes For some bytecodes it is beneficial to always look for a Star bytecode when dispatching to the next and inline perform it without dispatching to the Star handler. BUG=v8:4280 LOG=N ==========
klaasb@google.com changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
The CQ bit was checked by klaasb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset 5 has --predictable benchmark runs. On Patchset 6 (just formatting) there's an ongoing run without that flag.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9149) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
A few trivial nits, but looks good here. https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/byteco... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/byteco... src/interpreter/bytecodes.h:625: // Returns true if the handler should look ahead and inline a s/handler/handler for |bytecode|/ https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/byteco... src/interpreter/bytecodes.h:626: // dispatch to a Star bytecode s/bytecode/bytecode./ https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:588: Label store_register(this), dispatch(this, 2, merged_vars); Naming here might be better as store and store_done. Dispatch isn't being done here.
The changes to the InterpreterAssembler break the unit test for it. When inlining the Star and it's dispatch most of the assumptions don't hold anymore afterwards. https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/byteco... File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/byteco... src/interpreter/bytecodes.h:625: // Returns true if the handler should look ahead and inline a On 2016/07/15 15:30:51, oth wrote: > s/handler/handler for |bytecode|/ Done. https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/byteco... src/interpreter/bytecodes.h:626: // dispatch to a Star bytecode On 2016/07/15 15:30:51, oth wrote: > s/bytecode/bytecode./ Done. https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/100001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:588: Label store_register(this), dispatch(this, 2, merged_vars); On 2016/07/15 15:30:51, oth wrote: > Naming here might be better as store and store_done. Dispatch isn't being done > here. Done.
Thanks for addressing comments. One more that's more of a question than requiring work. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... src/interpreter/bytecodes.cc:575: case Bytecode::kLdaZero: Is there data driving the selected bytecodes here? Naively, it might seem that all binary operators would benefit from the optimization proposed here (?) and perhaps other bytecodes too. What happens if this done for all bytecodes?
https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... src/interpreter/bytecodes.cc:575: case Bytecode::kLdaZero: On 2016/07/18 08:05:40, oth wrote: > Is there data driving the selected bytecodes here? > > Naively, it might seem that all binary operators would benefit from the > optimization proposed here (?) and perhaps other bytecodes too. What happens if > this done for all bytecodes? Some experiments are in the earlier patchsets (although code has been refactored since then). Octane shows less improvement when this is enabled for more bytecodes and some of it regresses more. I've gathered data from speedometer, verge and facebook that show mostly the same set of byte codes often dispatching to Star and a similar list for Octane which shows a set with quite some differences (more Shift*Smi, Bitwise*Smi, StaKeyed*Sloppy). Most bytecodes don't dispatch to Star all that often, mostly an order of magnitude less than these. These are mostly the set that all agree on within the Top 20 bytecodes and mostly arithmetic bytecodes. From the benchmarks and dispatch data I don't think it would improve if done for all bytecodes. Should I put a comment here, so others know what to look for when adding a Bytecode here?
Overall looks good, a couple of comments. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... src/interpreter/bytecodes.cc:579: case Bytecode::kMov: kMov seems an unusual one since it doesn't update the accumulator (so I'm not sure what the Star would be doing here) - Could you check this one. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:563: Variable var_offset(this, MachineRepresentation::kTagged); The offset is not a tagged value, it is just a raw int. Making it kTagged would cause the GC to treat it like a tagged pointer, which could cause crashes). This should be a intptr, so MachineType::PointerRepresentation(). https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:564: Variable var_bytecode(this, MachineRepresentation::kWord32); kWord8, no? https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:574: void InterpreterAssembler::InlineDispatchToStar(Variable& target_bytecode, nit - StarDispatchLookahead https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:577: TraceBytecode(Runtime::kInterpreterTraceBytecodeExit); You are now doing this twice for the same dispatch in the case where you don't inline the Star dispatch (both here and in DispatchToBytecodeHandlerEntry). You should probably do this in the branch. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:580: bytecode_ = Bytecode::kStar; Could you split up the code which does the check for whether the next bytecode is a Star, and have a separate function which performs the actual inline Star. Could you make the bytecode_ set to Star only in this inline star function (saving and restoring AccumulatorUse for the original bytecode). https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:581: bytecode_offset_.Bind(target_offset.value()); Not sure why this is needed? https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:584: TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry); ditto https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:587: Variable* merged_vars[2] = {&target_offset, &target_bytecode}; I don't think you need these to list the merged variables on the done path, they should be auto-merged for you unless you do a backwards branch. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:609: Node* InterpreterAssembler::DispatchToLoaded(Node* target_bytecode, nit - DispatchToBytecode https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter.cc:373: __ StoreAccumulatorToRegister(); Not so keen on inlining this into the interpreter-assembler, since it depends on reading a bytecode operand for a particular bytecode (kStar). Could we either pass the register index in as a parameter, or just inline the code for both call sites?
https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/byteco... src/interpreter/bytecodes.cc:579: case Bytecode::kMov: On 2016/07/18 11:07:51, rmcilroy wrote: > kMov seems an unusual one since it doesn't update the accumulator (so I'm not > sure what the Star would be doing here) - Could you check this one. I thought about that too. It is the top 10-20 for all benchmarks and websites I looked at, though. Sometimes even higher up than LdaZero, Sub, SubSmi and always higher up than New and higher than Dec except on Octane. 11th most source of dispatch in Octane. I can have look whether it's storing to the same registers Mov is using, or whether it's just coincidence. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:563: Variable var_offset(this, MachineRepresentation::kTagged); On 2016/07/18 11:07:51, rmcilroy wrote: > The offset is not a tagged value, it is just a raw int. Making it kTagged would > cause the GC to treat it like a tagged pointer, which could cause crashes). This > should be a intptr, so MachineType::PointerRepresentation(). Done. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:564: Variable var_bytecode(this, MachineRepresentation::kWord32); On 2016/07/18 11:07:51, rmcilroy wrote: > kWord8, no? Done. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:574: void InterpreterAssembler::InlineDispatchToStar(Variable& target_bytecode, On 2016/07/18 11:07:51, rmcilroy wrote: > nit - StarDispatchLookahead Done. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:577: TraceBytecode(Runtime::kInterpreterTraceBytecodeExit); On 2016/07/18 11:07:51, rmcilroy wrote: > You are now doing this twice for the same dispatch in the case where you don't > inline the Star dispatch (both here and in DispatchToBytecodeHandlerEntry). You > should probably do this in the branch. Done. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:580: bytecode_ = Bytecode::kStar; On 2016/07/18 11:07:51, rmcilroy wrote: > Could you split up the code which does the check for whether the next bytecode > is a Star, and have a separate function which performs the actual inline Star. > Could you make the bytecode_ set to Star only in this inline star function > (saving and restoring AccumulatorUse for the original bytecode). Yes, this assumed that a single Dispatch call was basically the last thing the handler generation did. Currently none of the bytecodes with lookahead use a second call to Dispatch. I've refactored the "store"/inlining branch into a separate function together with the bytecode_, bytecode_offset_ switching and trace call generation. Dispatch will generate the same code multiple times - as it actually did before, it now also generates the Star inlining at the same time. We can just rewrite the handlers to make jumps to a single Dispatch (in another CL?), or generate a Label for the dispatch part and Goto to that? https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:581: bytecode_offset_.Bind(target_offset.value()); On 2016/07/18 11:07:51, rmcilroy wrote: > Not sure why this is needed? For BytecodeOffset() to have the correct value when calling BytecodeOperandReg(0) and Advance() when inlining Star and building the next dispatch. Doesn't actually need to Bind() though, refactored it together with the rest. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:584: TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry); On 2016/07/18 11:07:51, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:587: Variable* merged_vars[2] = {&target_offset, &target_bytecode}; On 2016/07/18 11:07:51, rmcilroy wrote: > I don't think you need these to list the merged variables on the done path, they > should be auto-merged for you unless you do a backwards branch. Done. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:609: Node* InterpreterAssembler::DispatchToLoaded(Node* target_bytecode, On 2016/07/18 11:07:51, rmcilroy wrote: > nit - DispatchToBytecode Done. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter.cc:373: __ StoreAccumulatorToRegister(); On 2016/07/18 11:07:52, rmcilroy wrote: > Not so keen on inlining this into the interpreter-assembler, since it depends on > reading a bytecode operand for a particular bytecode (kStar). Could we either > pass the register index in as a parameter, or just inline the code for both call > sites? Done. Inlined.
The CQ bit was checked by klaasb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2142273003/diff/180001/src/interpreter/byteco... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/2142273003/diff/180001/src/interpreter/byteco... src/interpreter/bytecodes.cc:596: case Bytecode::kForInPrepare: This one doesn't write the accumulator, but in all things I ran, it dispatched to Star in 100% of the cases. All of the new ones I added were found by looking at dispatch report with https://codereview.chromium.org/2159683003/ for facebook, verge, speedometer (on 53.0.2785.8) and Octane-noopt locally. They all dispatch always or nearly always to Star on all of them.
Looks good. A couple more comments, but once Advance and TraceBytecode is sorted, looks good to go. https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/120001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:580: bytecode_ = Bytecode::kStar; On 2016/07/18 15:15:29, klaasb wrote: > On 2016/07/18 11:07:51, rmcilroy wrote: > > Could you split up the code which does the check for whether the next bytecode > > is a Star, and have a separate function which performs the actual inline Star. > > Could you make the bytecode_ set to Star only in this inline star function > > (saving and restoring AccumulatorUse for the original bytecode). > > Yes, this assumed that a single Dispatch call was basically the last thing the > handler generation did. Currently none of the bytecodes with lookahead use a > second call to Dispatch. > > I've refactored the "store"/inlining branch into a separate function together > with the bytecode_, bytecode_offset_ switching and trace call generation. > Dispatch will generate the same code multiple times - as it actually did before, > it now also generates the Star inlining at the same time. > We can just rewrite the handlers to make jumps to a single Dispatch (in another > CL?), or generate a Label for the dispatch part and Goto to that? Let's not do this in this CL (see other comments). https://codereview.chromium.org/2142273003/diff/180001/src/interpreter/byteco... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/2142273003/diff/180001/src/interpreter/byteco... src/interpreter/bytecodes.cc:596: case Bytecode::kForInPrepare: On 2016/07/18 19:45:46, klaasb wrote: > This one doesn't write the accumulator, but in all things I ran, it dispatched > to Star in 100% of the cases. > > All of the new ones I added were found by looking at dispatch report with > https://codereview.chromium.org/2159683003/ for facebook, verge, speedometer (on > 53.0.2785.8) and Octane-noopt locally. They all dispatch always or nearly always > to Star on all of them. As discussed offline, let's remove the ones which are 100% of the time (at least 100% with REO disabled) and instead change the bytecodes to accept an output register, which would save on the dispatch and reduce code size. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (left): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:550: } This should also be in LoadBytecode (hopefully we can teach TF to avoid actually emitting any code for this on most platforms, but we should be consistent and have the bytecode always treated as word-sized (since it needs to be for the load below). https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:553: TraceBytecodeDispatch(target_bytecode); Just to point out, we will no longer count these inlined Star's as bytecode dispatches. This is probably what we want, but we should keep it in mind when looking at dispatch count reports in the future. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:565: TraceBytecode(Runtime::kInterpreterTraceBytecodeExit); I think the TraceBytecode can still stay in DispatchToBytecodeHandler. All you need to do is add another TraceBytecode(Runtime::kInterpreterTraceBytecodeExit) in StarDispatchLookahead (when exiting the current bytecode), then have the TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry) in InlineStar and depend on the exit in DispatchToBytecodeHandler for tracing the exit of Star. Note: The TraceBytecode relies on BytecodeOffset being correct, so you will need to update that in Advance() as mentioned above. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:578: Node* star_bytecode = Int32Constant(static_cast<int>(Bytecode::kStar)); IntPtrConstant https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:581: Bind(&store); nit - do_inline_star https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:594: bytecode_ = Bytecode::kStar; nit - newline https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.h (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:222: // Note: this does not update BytecodeOffset() itself. Now that you have a variable for bytecode_offset, could you have these update BytecodeOffset() too? That might mean you don't need to pass around target_offset as a variable to StarDispatchLookahead / InlineStar. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:243: void InlineStar(Variable& target_offset); nit - could you move these above DispatchTo (group the DispatchX methods together. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter.cc:1657: __ Dispatch(); Can we do this change separately (if at all) - as mentioned, the Dispatch in the fast-path is actually faster because there hasn't been a call before it happens (so doesn't need to reload the bytecode array), and since the slow path isn't run so often, the added instruction size doesn't necessarily hurt us. It should eventually be possible to make the second dispatch as fast as the first eventually, so feel free to add a TODO to make this change when that happens. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter.cc:1732: __ Dispatch(); Ditto (same will apply once we can inline FastNewSloppyArguments) https://codereview.chromium.org/2142273003/diff/200001/test/unittests/interpr... File test/unittests/interpreter/interpreter-assembler-unittest.cc (right): https://codereview.chromium.org/2142273003/diff/200001/test/unittests/interpr... test/unittests/interpreter/interpreter-assembler-unittest.cc:319: bool hasStarLookahead = underscore style for local variables (has_star_lookahead)
https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (left): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:550: } On 2016/07/19 10:03:56, rmcilroy wrote: > This should also be in LoadBytecode (hopefully we can teach TF to avoid actually > emitting any code for this on most platforms, but we should be consistent and > have the bytecode always treated as word-sized (since it needs to be for the > load below). Done. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:553: TraceBytecodeDispatch(target_bytecode); On 2016/07/19 10:03:56, rmcilroy wrote: > Just to point out, we will no longer count these inlined Star's as bytecode > dispatches. This is probably what we want, but we should keep it in mind when > looking at dispatch count reports in the future. Acknowledged. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:565: TraceBytecode(Runtime::kInterpreterTraceBytecodeExit); On 2016/07/19 10:03:56, rmcilroy wrote: > I think the TraceBytecode can still stay in DispatchToBytecodeHandler. All you > need to do is add another TraceBytecode(Runtime::kInterpreterTraceBytecodeExit) > in StarDispatchLookahead (when exiting the current bytecode), then have the > TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry) in InlineStar and depend > on the exit in DispatchToBytecodeHandler for tracing the exit of Star. > Note: The TraceBytecode relies on BytecodeOffset being correct, so you will need > to update that in Advance() as mentioned above. I had it that way before, but probably the offset was incorrect at some point and it printed the wrong acc/reg usage. It's somehow still wrong after doing the Advance - there's something I'm not understanding. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:578: Node* star_bytecode = Int32Constant(static_cast<int>(Bytecode::kStar)); On 2016/07/19 10:03:56, rmcilroy wrote: > IntPtrConstant Done. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:581: Bind(&store); On 2016/07/19 10:03:56, rmcilroy wrote: > nit - do_inline_star Done. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:594: bytecode_ = Bytecode::kStar; On 2016/07/19 10:03:56, rmcilroy wrote: > nit - newline Done. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.h (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:222: // Note: this does not update BytecodeOffset() itself. On 2016/07/19 10:03:56, rmcilroy wrote: > Now that you have a variable for bytecode_offset, could you have these update > BytecodeOffset() too? That might mean you don't need to pass around > target_offset as a variable to StarDispatchLookahead / InlineStar. Done. Need to re-bind the offset to the previous one at the end of every Dispatch since code generated after a first Dispatch expects to operate on the previous bytecode's offset, e.g. for operands or further dispatches. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:243: void InlineStar(Variable& target_offset); On 2016/07/19 10:03:56, rmcilroy wrote: > nit - could you move these above DispatchTo (group the DispatchX methods > together. Done. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter.cc:1657: __ Dispatch(); On 2016/07/19 10:03:56, rmcilroy wrote: > Can we do this change separately (if at all) - as mentioned, the Dispatch in the > fast-path is actually faster because there hasn't been a call before it happens > (so doesn't need to reload the bytecode array), and since the slow path isn't > run so often, the added instruction size doesn't necessarily hurt us. > > It should eventually be possible to make the second dispatch as fast as the > first eventually, so feel free to add a TODO to make this change when that > happens. Done. https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter.cc:1732: __ Dispatch(); On 2016/07/19 10:03:56, rmcilroy wrote: > Ditto (same will apply once we can inline FastNewSloppyArguments) Done. https://codereview.chromium.org/2142273003/diff/200001/test/unittests/interpr... File test/unittests/interpreter/interpreter-assembler-unittest.cc (right): https://codereview.chromium.org/2142273003/diff/200001/test/unittests/interpr... test/unittests/interpreter/interpreter-assembler-unittest.cc:319: bool hasStarLookahead = On 2016/07/19 10:03:56, rmcilroy wrote: > underscore style for local variables (has_star_lookahead) Done.
https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:565: TraceBytecode(Runtime::kInterpreterTraceBytecodeExit); On 2016/07/19 14:24:13, klaasb wrote: > On 2016/07/19 10:03:56, rmcilroy wrote: > > I think the TraceBytecode can still stay in DispatchToBytecodeHandler. All you > > need to do is add another > TraceBytecode(Runtime::kInterpreterTraceBytecodeExit) > > in StarDispatchLookahead (when exiting the current bytecode), then have the > > TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry) in InlineStar and > depend > > on the exit in DispatchToBytecodeHandler for tracing the exit of Star. > > Note: The TraceBytecode relies on BytecodeOffset being correct, so you will > need > > to update that in Advance() as mentioned above. > > > I had it that way before, but probably the offset was incorrect at some point > and it printed the wrong acc/reg usage. > It's somehow still wrong after doing the Advance - there's something I'm not > understanding. As discussed offline, this is probably because you call Advance before TraceBytecodeExit, which makes BytecodeOffset wrong for TraceBytecodeExit. Maybe we could just move TraceBytecodeExit before (or into) the Advance?
https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/200001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:565: TraceBytecode(Runtime::kInterpreterTraceBytecodeExit); On 2016/07/19 15:26:04, rmcilroy wrote: > On 2016/07/19 14:24:13, klaasb wrote: > > On 2016/07/19 10:03:56, rmcilroy wrote: > > > I think the TraceBytecode can still stay in DispatchToBytecodeHandler. All > you > > > need to do is add another > > TraceBytecode(Runtime::kInterpreterTraceBytecodeExit) > > > in StarDispatchLookahead (when exiting the current bytecode), then have the > > > TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry) in InlineStar and > > depend > > > on the exit in DispatchToBytecodeHandler for tracing the exit of Star. > > > Note: The TraceBytecode relies on BytecodeOffset being correct, so you will > > need > > > to update that in Advance() as mentioned above. > > > > > > I had it that way before, but probably the offset was incorrect at some point > > and it printed the wrong acc/reg usage. > > It's somehow still wrong after doing the Advance - there's something I'm not > > understanding. > > As discussed offline, this is probably because you call Advance before > TraceBytecodeExit, which makes BytecodeOffset wrong for TraceBytecodeExit. Maybe > we could just move TraceBytecodeExit before (or into) the Advance? Yes, that's what I did now.
Couple of last comments. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:533: bytecode_offset_.Bind(previous_offset); I don't think we need this line do we? We are be exiting this basic block, so I don't think the updates we've done to bytecode_offset_ will apply in any future Dispatch in a different basic block. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:565: void InterpreterAssembler::StarDispatchLookahead(Variable& target_bytecode) { no non-const references - https://google.github.io/styleguide/cppguide.html#Reference_Arguments Make this a pointer instead, or (possibly better), just pass in the target_bytecode as a node, and return a node with the (possibly updated) bytecode, and keep all the Variable logic contained in this function. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:571: Bind(&do_inline_star); nit - newline above. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:610: bytecode_offset_.Bind(previous_offset); same comment as above. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:615: Node* target_bytecode = LoadBytecode(new_bytecode_offset); nit - DCHECK here that !Bytecodes::IsStarLookahead(bytecode_, operand_scale_). https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:659: } Remove extra ChangeUint32ToUint64 given it is already done in LoadBytecode https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... File src/interpreter/interpreter-assembler.h (right): https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:260: CodeStubAssembler::Variable bytecode_offset_; nit - move one down with other variables.
https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:533: bytecode_offset_.Bind(previous_offset); On 2016/07/19 20:18:27, rmcilroy wrote: > I don't think we need this line do we? We are be exiting this basic block, so I > don't think the updates we've done to bytecode_offset_ will apply in any future > Dispatch in a different basic block. Done. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:565: void InterpreterAssembler::StarDispatchLookahead(Variable& target_bytecode) { On 2016/07/19 20:18:27, rmcilroy wrote: > no non-const references - > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > Make this a pointer instead, or (possibly better), just pass in the > target_bytecode as a node, and return a node with the (possibly updated) > bytecode, and keep all the Variable logic contained in this function. Done. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:571: Bind(&do_inline_star); On 2016/07/19 20:18:27, rmcilroy wrote: > nit - newline above. Done. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:610: bytecode_offset_.Bind(previous_offset); On 2016/07/19 20:18:27, rmcilroy wrote: > same comment as above. Done. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:615: Node* target_bytecode = LoadBytecode(new_bytecode_offset); On 2016/07/19 20:18:27, rmcilroy wrote: > nit - DCHECK here that !Bytecodes::IsStarLookahead(bytecode_, operand_scale_). Done. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:659: } On 2016/07/19 20:18:27, rmcilroy wrote: > Remove extra ChangeUint32ToUint64 given it is already done in LoadBytecode Done. https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... File src/interpreter/interpreter-assembler.h (right): https://codereview.chromium.org/2142273003/diff/240001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:260: CodeStubAssembler::Variable bytecode_offset_; On 2016/07/19 20:18:27, rmcilroy wrote: > nit - move one down with other variables. Done.
Description was changed from ========== [interpreter] Inline Star on dispatch for some bytecodes For some bytecodes it is beneficial to always look for a Star bytecode when dispatching to the next and inline perform it without dispatching to the Star handler. BUG=v8:4280 LOG=N ========== to ========== [interpreter] Inline Star on dispatch for some bytecodes For some bytecodes it is beneficial to always look for a Star bytecode when dispatching to the next and inline perform it without dispatching to the Star handler. BUG=v8:4280 LOG=N ==========
LGTM once final comments are addressed, thanks. https://codereview.chromium.org/2142273003/diff/260001/src/interpreter/interp... File src/interpreter/interpreter-assembler.h (right): https://codereview.chromium.org/2142273003/diff/260001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:234: // to, to |target_bytecode| and its offset to |target_offset|. Update comment, remove duplicate "to" and also mention that the new target_bytecode is returned https://codereview.chromium.org/2142273003/diff/260001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:238: // next offset after Star Update comment https://codereview.chromium.org/2142273003/diff/260001/test/unittests/interpr... File test/unittests/interpreter/interpreter-assembler-unittest.cc (right): https://codereview.chromium.org/2142273003/diff/260001/test/unittests/interpr... test/unittests/interpreter/interpreter-assembler-unittest.cc:320: interpreter::Bytecodes::IsStarLookahead(bytecode, operand_scale); nit - just inline this into if block below
PTAL, see comments https://codereview.chromium.org/2142273003/diff/260001/src/interpreter/interp... File src/interpreter/interpreter-assembler.h (right): https://codereview.chromium.org/2142273003/diff/260001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:234: // to, to |target_bytecode| and its offset to |target_offset|. On 2016/07/20 10:34:12, rmcilroy wrote: > Update comment, remove duplicate "to" and also mention that the new > target_bytecode is returned Done. https://codereview.chromium.org/2142273003/diff/260001/src/interpreter/interp... src/interpreter/interpreter-assembler.h:238: // next offset after Star On 2016/07/20 10:34:12, rmcilroy wrote: > Update comment Done. https://codereview.chromium.org/2142273003/diff/260001/test/unittests/interpr... File test/unittests/interpreter/interpreter-assembler-unittest.cc (right): https://codereview.chromium.org/2142273003/diff/260001/test/unittests/interpr... test/unittests/interpreter/interpreter-assembler-unittest.cc:320: interpreter::Bytecodes::IsStarLookahead(bytecode, operand_scale); On 2016/07/20 10:34:12, rmcilroy wrote: > nit - just inline this into if block below Done. https://codereview.chromium.org/2142273003/diff/300001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/300001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:535: return DispatchToBytecode(target_bytecode, new_bytecode_offset); The DCHECK was in DispatchTo, which was only called here. It effectively forbids Jump for IsStarLookahead bytecodes, so I inlined it here as all other things DispatchTo did before moved to DispatchToBytecode and LoadBytecode. https://codereview.chromium.org/2142273003/diff/300001/test/unittests/interpr... File test/unittests/interpreter/interpreter-assembler-unittest.cc (right): https://codereview.chromium.org/2142273003/diff/300001/test/unittests/interpr... test/unittests/interpreter/interpreter-assembler-unittest.cc:384: Jump was the only caller of DispatchTo, to which I added the DCHECK - which breaks this test. I'm not sure this test is really that useful to do for every bytecode?
Still LGTM. https://codereview.chromium.org/2142273003/diff/300001/src/interpreter/interp... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2142273003/diff/300001/src/interpreter/interp... src/interpreter/interpreter-assembler.cc:535: return DispatchToBytecode(target_bytecode, new_bytecode_offset); On 2016/07/20 11:22:55, klaasb wrote: > The DCHECK was in DispatchTo, which was only called here. > It effectively forbids Jump for IsStarLookahead bytecodes, so I inlined it here > as all other things DispatchTo did before moved to DispatchToBytecode and > LoadBytecode. Works for me, thanks. https://codereview.chromium.org/2142273003/diff/300001/test/unittests/interpr... File test/unittests/interpreter/interpreter-assembler-unittest.cc (right): https://codereview.chromium.org/2142273003/diff/300001/test/unittests/interpr... test/unittests/interpreter/interpreter-assembler-unittest.cc:384: On 2016/07/20 11:22:55, klaasb wrote: > Jump was the only caller of DispatchTo, to which I added the DCHECK - which > breaks this test. > I'm not sure this test is really that useful to do for every bytecode? Agreed - could you instead make the check: if (!interpreter::Bytecodes::IsJump(bytecode)) return;
The CQ bit was checked by klaasb@google.com
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/2142273003/#ps320001 (title: "restrict InterpreterAssemblerTest.Jump to jumps")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [interpreter] Inline Star on dispatch for some bytecodes For some bytecodes it is beneficial to always look for a Star bytecode when dispatching to the next and inline perform it without dispatching to the Star handler. BUG=v8:4280 LOG=N ========== to ========== [interpreter] Inline Star on dispatch for some bytecodes For some bytecodes it is beneficial to always look for a Star bytecode when dispatching to the next and inline perform it without dispatching to the Star handler. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== [interpreter] Inline Star on dispatch for some bytecodes For some bytecodes it is beneficial to always look for a Star bytecode when dispatching to the next and inline perform it without dispatching to the Star handler. BUG=v8:4280 LOG=N ========== to ========== [interpreter] Inline Star on dispatch for some bytecodes For some bytecodes it is beneficial to always look for a Star bytecode when dispatching to the next and inline perform it without dispatching to the Star handler. BUG=v8:4280 LOG=N Committed: https://crrev.com/5f603e838a731ed5e3585d94c5b0889ab749f24b Cr-Commit-Position: refs/heads/master@{#37904} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/5f603e838a731ed5e3585d94c5b0889ab749f24b Cr-Commit-Position: refs/heads/master@{#37904} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
