Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1684113002:
[Interpreter] Handle negative ints in generate-bytecode-expectations. (Closed)
|
Created:
4 years, 10 months ago by Stefano Sanfilippo Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com, oth, rmcilroy Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] Handle negative ints in generate-bytecode-expectations.
The previous implementation used GetRawOperand(), which allows a nicely
unified handling of all scalar types, but returns an unsigned type.
Because of this, generate-bytecode-expectations couldn't properly handle
negative numbers.
This commit differentiate between different types of scalar operands and
uses the appropriate getter from i::interpreter::BytecodeArrayIterator,
thus correctly handling signed types where needed.
Two new helpers have been added to i::interpreter::Bytecodes:
* IsImmediateOperandType()
* IsIndexOperandType()
with the intuitive semantic.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/8bfd4a5ac907c3ca1a68ccff181b88b9707dff67
Cr-Commit-Position: refs/heads/master@{#33874}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Reorder methods in i::interpreter::Bytecodes. #
Total comments: 2
Patch Set 3 : Move IsImmediateOperandType. #
Messages
Total messages: 23 (12 generated)
ssanfilippo@chromium.org changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
Apparently GetRawOperand does not play nice with negative numbers, so here's the fix. PTAL.
Description was changed from ========== [Interpreter] handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative number properly. This commit differentiate between different types of scalar operands and uses the appropriate getter from the BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative number properly. This commit differentiate between different types of scalar operands and uses the appropriate getter from the BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative number properly. This commit differentiate between different types of scalar operands and uses the appropriate getter from the BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative number properly. This commit differentiate between different types of scalar operands and uses the appropriate getter from the BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative number properly. This commit differentiate between different types of scalar operands and uses the appropriate getter from the BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from the BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from the BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from i::interpreter::BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from i::interpreter::BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from i::interpreter::BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ==========
Looks good, a couple of nits and a request please. https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.cc File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:339: } nit - move above IsMaybeRegisterOperandType (and move IsMaybeRegisterOperandType below IsRegisterCountOperandType) https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.h File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.h... src/interpreter/bytecodes.h:475: static bool IsIndexOperandType(OperandType operand_type); nit - Could you rearrange the order of these functions to be in the order: IsIndexOperandType IsImmediateOperandType IsRegisterCountOperandType IsRegisterOperandType IsRegisterInputOperandType IsRegisterOutputOperandType IsMaybeRegisterOperandType https://codereview.chromium.org/1684113002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1684113002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:155: stream << bytecode_iter.GetCountOperand(op_index); Could you rename BytecodeIterator::GetCountOperand to GetRegisterCountOperand as well please?
The CQ bit was checked by rmcilroy@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/1684113002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684113002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/13549)
https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.cc File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.c... src/interpreter/bytecodes.cc:339: } On 2016/02/10 15:26:12, rmcilroy wrote: > nit - move above IsMaybeRegisterOperandType (and move IsMaybeRegisterOperandType > below IsRegisterCountOperandType) Done. https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.h File src/interpreter/bytecodes.h (right): https://codereview.chromium.org/1684113002/diff/1/src/interpreter/bytecodes.h... src/interpreter/bytecodes.h:475: static bool IsIndexOperandType(OperandType operand_type); On 2016/02/10 15:26:12, rmcilroy wrote: > nit - Could you rearrange the order of these functions to be in the order: > IsIndexOperandType > IsImmediateOperandType > IsRegisterCountOperandType > IsRegisterOperandType > IsRegisterInputOperandType > IsRegisterOutputOperandType > IsMaybeRegisterOperandType Done.
https://codereview.chromium.org/1684113002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1684113002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:155: stream << bytecode_iter.GetCountOperand(op_index); On 2016/02/10 15:26:12, rmcilroy wrote: > Could you rename BytecodeIterator::GetCountOperand to GetRegisterCountOperand as > well please? Done. I have prepared a different CL, since this change is not tightly related to this one: https://codereview.chromium.org/1691433002/
lgtm, thanks. https://codereview.chromium.org/1684113002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1684113002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:339: bool Bytecodes::IsImmediateOperandType(OperandType operand_type) { nit - move above IsRegisterCountOperandType
https://codereview.chromium.org/1684113002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecodes.cc (right): https://codereview.chromium.org/1684113002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecodes.cc:339: bool Bytecodes::IsImmediateOperandType(OperandType operand_type) { On 2016/02/10 16:43:14, rmcilroy wrote: > nit - move above IsRegisterCountOperandType Done.
The CQ bit was checked by ssanfilippo@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/1684113002/#ps40001 (title: "Move IsImmediateOperandType.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684113002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684113002/40001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from i::interpreter::BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from i::interpreter::BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from i::interpreter::BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Handle negative ints in generate-bytecode-expectations. The previous implementation used GetRawOperand(), which allows a nicely unified handling of all scalar types, but returns an unsigned type. Because of this, generate-bytecode-expectations couldn't properly handle negative numbers. This commit differentiate between different types of scalar operands and uses the appropriate getter from i::interpreter::BytecodeArrayIterator, thus correctly handling signed types where needed. Two new helpers have been added to i::interpreter::Bytecodes: * IsImmediateOperandType() * IsIndexOperandType() with the intuitive semantic. BUG=v8:4280 LOG=N Committed: https://crrev.com/8bfd4a5ac907c3ca1a68ccff181b88b9707dff67 Cr-Commit-Position: refs/heads/master@{#33874} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8bfd4a5ac907c3ca1a68ccff181b88b9707dff67 Cr-Commit-Position: refs/heads/master@{#33874} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
