Hi Danno,
I think we are getting there, PTAL, thx!
--Michael
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs-hydrogen.cc#newcode42
src/code-stubs-hydrogen.cc:42: int parameter_count =
descriptor_->register_param_count() - 1;
On 2014/07/16 13:19:59, danno wrote:
GetEnvironmentParameterCount
Done.
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs-hydrogen.cc#newcode122
src/code-stubs-hydrogen.cc:122: int param_count =
descriptor_->register_param_count() - 1;
On 2014/07/16 13:19:59, danno wrote:
Use GetEnvironmentLength(), then comment is unnecessary
Done. (but I used GetEnvironmentParameterCount() as suggested in the
line 42 comment)
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs-hydrogen.cc#newcode132
src/code-stubs-hydrogen.cc:132: int parameter_index = i+1;
On 2014/07/16 13:19:59, danno wrote:
nit: make sure to clang format your patch. Comment about accounting
for the
context parameter?
I got rid of this whole notion of "convert a parameter index to a
descriptor register parameter index" with the +1 thing. I just made a
new method GetEnvironmentParameterRegister() and
GetEnvironmentParameterRepresentation().
After addressing the feedback to do something like that in
deoptimizer.cc, it made sense to come back and touch this too.
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs.h#newcode276
src/code-stubs.h:276: bool initialized() const { return
register_param_count_ >= 0; }
On 2014/07/16 13:19:59, danno wrote:
style: should be IsInitialized()
Done.
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs.h#newcode280
src/code-stubs.h:280: }
On 2014/07/16 13:19:59, danno wrote:
I think with a little more "duplication", you can make this clearer.
In addition
to GetEnvironmentLength, you should have GetEnvironmentParameterCount
(which
should actually be register_param_count_ - 1), which is used really
where you
current -1 due to the smaller environment, and there you shouldn't
have to do
the math.
I fully factored this out so that customers who prefer to think of
parameters without the context register have methods to do so, and
customers who need the full set (including context) can have what they
want. Nobody has to do adjustments like "+1" and "-1" any more to deal
with the context register.
https://codereview.chromium.org/384403002/diff/130001/src/code-stubs.h#newcode282
src/code-stubs.h:282: int register_param_count() const { return
register_param_count_; }
On 2014/07/16 13:19:59, danno wrote:
For consistency, make this GetRegisterParameterCount()
Done.
https://codereview.chromium.org/384403002/diff/130001/src/deoptimizer.cc
File src/deoptimizer.cc (right):
https://codereview.chromium.org/384403002/diff/130001/src/deoptimizer.cc#newcode1559
src/deoptimizer.cc:1559: int param_count =
descriptor->register_param_count() - 1;
On 2014/07/16 13:19:59, danno wrote:
Use GetEnvironmentParameterLength(), then comment is unnecessary
Done.
https://codereview.chromium.org/384403002/diff/130001/src/deoptimizer.cc#newcode1711
src/deoptimizer.cc:1711: // the descriptor parameter array accordingly.
On 2014/07/16 13:19:59, danno wrote:
Why not move this up to the loop iteration? e.g.
for (int i = 1; i < param_count; ++i) {
Otherwise, I think you might actually do one too many iterations here,
since you
access param_count + 1, which is technically OOB
Well, I would have to make the terminating condition "i <= param_count",
because param count runs from [0, param_count] but the index in the
descriptor runs from [1, param_count+1] since we have to skip over the
context register. I thought about that and decided to keep the loop
condition simple, and the complexity in this notion that I need to
translate to a descriptor parameter register.
So I'd prefer to leave it this way, but I could go from 1 <= i <=
param_count too, let me know.
<EDIT>
On second thought, what a complex mess. I just introduced method
IsEnvironmentParameterCountRegister() which makes things simpler.
https://codereview.chromium.org/384403002/diff/130001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):
https://codereview.chromium.org/384403002/diff/130001/src/ia32/lithium-ia32.h#newcode1869
src/ia32/lithium-ia32.h:1869: :
inputs_(descriptor->GetEnvironmentLength() + 1, zone) {
On 2014/07/16 13:19:59, danno wrote:
This (and below and on other platforms should get
GetRegisterParameterCount() +
1
Done.
https://codereview.chromium.org/384403002/
--
--
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.
