Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1510553002:
[Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block. (Closed)
Description[Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block.
PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not
in the same basicblock. Changed it to be instantiated only when the previous bytecode
is in the same basic block.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/c03ac1e153efa01d1ed91eecfda22aefe1f453bf
Cr-Commit-Position: refs/heads/master@{#32702}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Changes PreviousBytecodeHelper to be instantiated only when the last bytecode is in the same basicb… #
Total comments: 16
Patch Set 3 : Addressed review comments #Patch Set 4 : Fixed a comment I forgot to fix in the last patch. #
Total comments: 4
Patch Set 5 : Address review comments. #Messages
Total messages: 19 (8 generated)
mythria@chromium.org changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
Could you please review my changes. This fixes one of the comments by Ross on this cl https://codereview.chromium.org/1468003002/ As discussed offline, I will have another cl to fix other comments. Thanks, Mythri
Thanks, this change is okay, but I think it'd safer to do as described below. https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... src/interpreter/bytecode-array-builder.cc:14: : array_builder_(array_builder) {} It would be better to have: CHECK(array_builder->LastBytecodeIsInSameBlock()) in the constructor and DCHECK in the getters, leaving GetBytecode and GetOperand alone. The code would be more readable and realiable if it explicitly checked the validity of the previous bytecode before instantiating PreviousBytecodeHelper. The PreviousBytecode class should have DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... src/interpreter/bytecode-array-builder.cc:16: bool GetBytecode(Bytecode* bytecode) const { Propose leaving this as Bytecode, adding MUST_USE_RESULT, and DCHECK'ing in the same block here. https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... src/interpreter/bytecode-array-builder.cc:27: uint32_t GetOperand(int operand_index) const { Propse adding MUST_USE_RESULT and DCHECK'ing in the same block here.
Description was changed from ========== [Interpreter] Fixes previous bytecode helper to return a boolean value on failure. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to return a boolean value to indicate if the previous bytecode is in the same basicblock. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixed PreviousBytecodeHelper to be used only if the previous bytecode is in the same basicblock. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basicblock. BUG=v8:4280 LOG=N ==========
Thanks for the comment Orion, code looks better now. Could you please take a look. Thanks, Mythri https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... src/interpreter/bytecode-array-builder.cc:14: : array_builder_(array_builder) {} On 2015/12/08 09:54:14, oth wrote: > It would be better to have: > > CHECK(array_builder->LastBytecodeIsInSameBlock()) > > in the constructor and DCHECK in the getters, leaving GetBytecode and GetOperand > alone. The code would be more readable and realiable if it explicitly checked > the validity of the previous bytecode before instantiating > PreviousBytecodeHelper. > > The PreviousBytecode class should have DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... src/interpreter/bytecode-array-builder.cc:16: bool GetBytecode(Bytecode* bytecode) const { On 2015/12/08 09:54:15, oth wrote: > Propose leaving this as Bytecode, adding MUST_USE_RESULT, and DCHECK'ing in the > same block here. Done. https://codereview.chromium.org/1510553002/diff/1/src/interpreter/bytecode-ar... src/interpreter/bytecode-array-builder.cc:27: uint32_t GetOperand(int operand_index) const { On 2015/12/08 09:54:14, oth wrote: > Propse adding MUST_USE_RESULT and DCHECK'ing in the same block here. The DCHECK is done when we call GetBytecode() from this function. So did not add another one here. Done.
Thanks Mythri, lgtm.
https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:16: // in the same basicblock. /s/basicblock/basic block/ throughout, and also in CL title and description. Also, please update the CLs title to be the same as the first line of the description and add newlines in the description to keep it < 80 characters. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:17: CHECK(array_builder_.LastBytecodeInSameBlock()); DCHECK is fine here I think. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:21: // lastbytecode is in the same basicblock. /s/lastbytecode/last bytecode/. Actually, there is no need for the second sentence since it is covered in the constructor. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:23: DCHECK(array_builder_.LastBytecodeInSameBlock()); Rather than DCHECKING this (which is already CHECKED above), could we instead stash array_builder_.last_bytecode_start_ in the constructor, and DCHECK that it is still equal here. This has the added benefit of ensuring that GetBytecode and GetOperand always refer to the same bytecode for the time when this helper was constructed. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:29: // that the previous bytecode is in the same basicblock. ditto for the last sentence. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:59: DISALLOW_COPY_AND_ASSIGN(PreviousBytecodeHelper); nit - newline above https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:656: if (!LastBytecodeInSameBlock()) { If you reverse this (do the check for if (LastBytecodeInSameBlock() { PreviousBytecode... }) you only need the call to Output(Bytecode::kToName) once. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:1106: if (!LastBytecodeInSameBlock()) { ditto (avoids double return false)
Description was changed from ========== [Interpreter] Fixed PreviousBytecodeHelper to be used only if the previous bytecode is in the same basicblock. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basicblock. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basicblock. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basicblock. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basicblock. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basicblock. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basic block. BUG=v8:4280 LOG=N ==========
I fixed the comments. Could you please take a look. Thanks, Mythri https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:16: // in the same basicblock. On 2015/12/08 13:43:02, rmcilroy wrote: > /s/basicblock/basic block/ throughout, and also in CL title and description. > > Also, please update the CLs title to be the same as the first line of the > description and add newlines in the description to keep it < 80 characters. Done. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:17: CHECK(array_builder_.LastBytecodeInSameBlock()); On 2015/12/08 13:43:02, rmcilroy wrote: > DCHECK is fine here I think. Done. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:21: // lastbytecode is in the same basicblock. On 2015/12/08 13:43:02, rmcilroy wrote: > /s/lastbytecode/last bytecode/. Actually, there is no need for the second > sentence since it is covered in the constructor. Done. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:23: DCHECK(array_builder_.LastBytecodeInSameBlock()); On 2015/12/08 13:43:02, rmcilroy wrote: > Rather than DCHECKING this (which is already CHECKED above), could we instead > stash array_builder_.last_bytecode_start_ in the constructor, and DCHECK that it > is still equal here. This has the added benefit of ensuring that GetBytecode and > GetOperand always refer to the same bytecode for the time when this helper was > constructed. Done. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:29: // that the previous bytecode is in the same basicblock. On 2015/12/08 13:43:02, rmcilroy wrote: > ditto for the last sentence. Done. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:59: DISALLOW_COPY_AND_ASSIGN(PreviousBytecodeHelper); On 2015/12/08 13:43:02, rmcilroy wrote: > nit - newline above Done. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:656: if (!LastBytecodeInSameBlock()) { On 2015/12/08 13:43:02, rmcilroy wrote: > If you reverse this (do the check for if (LastBytecodeInSameBlock() { > PreviousBytecode... }) you only need the call to Output(Bytecode::kToName) once. Done. https://codereview.chromium.org/1510553002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:1106: if (!LastBytecodeInSameBlock()) { On 2015/12/08 13:43:02, rmcilroy wrote: > ditto (avoids double return false) Done.
lgtm, thanks. https://codereview.chromium.org/1510553002/diff/30002/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1510553002/diff/30002/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:23: DCHECK(array_builder_.last_bytecode_start_ == previous_bytecode_start_); DCHECK_EQ https://codereview.chromium.org/1510553002/diff/30002/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:31: DCHECK(array_builder_.last_bytecode_start_ == previous_bytecode_start_); ditto
Thanks for your reviews. https://codereview.chromium.org/1510553002/diff/30002/src/interpreter/bytecod... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1510553002/diff/30002/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:23: DCHECK(array_builder_.last_bytecode_start_ == previous_bytecode_start_); On 2015/12/08 15:25:50, rmcilroy wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1510553002/diff/30002/src/interpreter/bytecod... src/interpreter/bytecode-array-builder.cc:31: DCHECK(array_builder_.last_bytecode_start_ == previous_bytecode_start_); On 2015/12/08 15:25:50, rmcilroy wrote: > ditto Done.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1510553002/#ps70001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510553002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510553002/70001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basic block. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basic block. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basic block. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block. PreviousBytecodeHelper used to return Bytecode::kLast if the previous bytecode was not in the same basicblock. Changed it to be instantiated only when the previous bytecode is in the same basic block. BUG=v8:4280 LOG=N Committed: https://crrev.com/c03ac1e153efa01d1ed91eecfda22aefe1f453bf Cr-Commit-Position: refs/heads/master@{#32702} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c03ac1e153efa01d1ed91eecfda22aefe1f453bf Cr-Commit-Position: refs/heads/master@{#32702} |
