Thanks. Exposed Register in bytecodes.h. I didn't like it when writing
PS1/2,
but it is a bit cleaner.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecode-array-builder.h#newcode93
src/interpreter/bytecode-array-builder.h:93: class Register {
On 2015/09/02 11:42:41, rmcilroy wrote:
Would it be simpler just to move the Register class over to
bytecodes.h?
(Genuine question)
Done.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode123
src/interpreter/bytecodes.cc:123: uint8_t
Bytecodes::ParameterIndexToOperand(int index, int parameter_count) {
On 2015/09/02 11:42:42, rmcilroy wrote:
Could we make this a Register::FromParameterIndex() instead (and then
can use
Register::ToOperand if we need the operand)? This is assuming we can
move
Register class over to bytecodes.h as in my earlier comment.
Done.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode126
src/interpreter/bytecodes.cc:126: DCHECK_GE(index, 0);
On 2015/09/02 11:42:41, rmcilroy wrote:
nit - DCHECK_LT(index, parameter_count)
Done.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode144
src/interpreter/bytecodes.cc:144: return 128 + kLastParamRegisterIndex;
On 2015/09/02 11:42:41, rmcilroy wrote:
This is a bit confusing. Could you use kMinRegisterIndex with the -1
adjustment
(from count to index) and mention that kLastParamRegisterIndex is
negative.
Comment added.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode186
src/interpreter/bytecodes.cc:186: os << "this";
On 2015/09/02 11:42:42, rmcilroy wrote:
nit - could we make it "<this>" or "|this|" or something to make it
clear this
is a parameter? Maybe do the same with "<a0>" or "|a0|"as well?
Done <this>.
Prefer current consistency of r0, r1, r2, ...a0, a1, etc.
https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc
File test/unittests/interpreter/bytecodes-unittest.cc (right):
https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc#newcode24
test/unittests/interpreter/bytecodes-unittest.cc:24: int
parameter_counts[] = {7, 13, 99};
On 2015/09/02 11:42:42, rmcilroy wrote:
nit - could you just use a std::vector here instead of having to
define
COUNT_OF?
Done. With uniform initialization, this is a fine idea (std::array
even). Chrome rules seem to be against it, but see it in use elsewhere
in v8 tests.
https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc#newcode43
test/unittests/interpreter/bytecodes-unittest.cc:43: for (int i = 0; i <
128; i++) {
On 2015/09/02 11:42:42, rmcilroy wrote:
nit - /s/128/kMaxRegisterIndex
Done.
https://codereview.chromium.org/1325983002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
