StubCallInterfaceDescriptor takes a context register.

StubCallInterfaceDescriptor and CallInterfaceDescriptor are unified under a base class InterfaceDescriptor.

Handling of the context register had to be massaged to effect the unification. This will make it easier to call hydrogen code stubs directly from crankshaft.

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22448

[mvstanton, 2014-07-14 12:53:08]

Hi Danno,
Here is the change we discussed, a way to merge CodeInterfaceDescriptor and
CodeStubInterfaceDescriptor: the use of a base class, and making context
implicit in CodeInterfaceDescriptor (as it is already in
CodeStubInterfaceDescriptor).

This should make my changes to fix bug v8:3417 less complex.
Thanks for the look,
--Michael

[danno, 2014-07-15 07:48:31]

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode274
src/code-stubs.h:274: class InterfaceDescriptor {
In general, could more stuff in this class be private?

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode276
src/code-stubs.h:276: static const Register ContextRegister();
Can you make this a virtual register that returns either the register if needed
or no_reg. That would obviate the need for needs_context_register().

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode283
src/code-stubs.h:283: int environment_length() const {
Since this is not an accessor with an indentical name, this should be
GetEnvironmentLength().

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode302
src/code-stubs.h:302: Representation* register_param_representations() const {
Does this have to be public?

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode306
src/code-stubs.h:306: virtual bool needs_context_register() const { return true;
}
NeedsContextRegister

https://codereview.chromium.org/384403002/diff/30001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/384403002/diff/30001/src/hydrogen-instruction...
src/hydrogen-instructions.h:2345: return descriptor_->needs_context_register() ?
2 : 1;
Is it possible to roll this into the descriptor, too? You might need two calls:
GetParameterRepresentation and  GetOperandParameterRepresentation that do the
implicit +1 if there is a context register. But that moves the logic out of this
call class.

https://codereview.chromium.org/384403002/diff/30001/src/hydrogen-instruction...
src/hydrogen-instructions.h:2413: return descriptor->environment_length() +
extra_operands;
Similar to GetParameterRepresentation, you will need two version of
GetEnvironmentLength

[mvstanton, 2014-07-16 09:43:26]

Hi Danno,
Okay, here is the "full circle" back around to specifying the context register
for all InterfaceDescriptors.

I'm pretty happy with this, but PTAL and let me know your thoughts, thanks!
--Michael

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode283
src/code-stubs.h:283: int environment_length() const {
On 2014/07/15 07:48:31, danno wrote:
> Since this is not an accessor with an indentical name, this should be
> GetEnvironmentLength().

Done.

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode302
src/code-stubs.h:302: Representation* register_param_representations() const {
On 2014/07/15 07:48:31, danno wrote:
> Does this have to be public?

No, it is gone, good catch.

https://codereview.chromium.org/384403002/diff/30001/src/code-stubs.h#newcode306
src/code-stubs.h:306: virtual bool needs_context_register() const { return true;
}
On 2014/07/15 07:48:31, danno wrote:
> NeedsContextRegister

Done.

[danno, 2014-07-16 13:19:59]

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...
src/code-stubs-hydrogen.cc:42: int parameter_count =
descriptor_->register_param_count() - 1;
GetEnvironmentParameterCount

https://codereview.chromium.org/384403002/diff/130001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:122: int param_count =
descriptor_->register_param_count() - 1;
Use GetEnvironmentLength(), then comment is unnecessary

https://codereview.chromium.org/384403002/diff/130001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:132: int parameter_index = i+1;
nit: make sure to clang format your patch. Comment about accounting for the
context parameter?

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#newcod...
src/code-stubs.h:276: bool initialized() const { return register_param_count_ >=
0; }
style: should be IsInitialized()

https://codereview.chromium.org/384403002/diff/130001/src/code-stubs.h#newcod...
src/code-stubs.h:280: }
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.

https://codereview.chromium.org/384403002/diff/130001/src/code-stubs.h#newcod...
src/code-stubs.h:282: int register_param_count() const { return
register_param_count_; }
For consistency, make this GetRegisterParameterCount()

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#newc...
src/deoptimizer.cc:1559: int param_count = descriptor->register_param_count() -
1;
Use GetEnvironmentParameterLength(), then comment is unnecessary

https://codereview.chromium.org/384403002/diff/130001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1711: // the descriptor parameter array accordingly.
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

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...
src/ia32/lithium-ia32.h:1869: : inputs_(descriptor->GetEnvironmentLength() + 1,
zone) {
This (and below and on other platforms should get GetRegisterParameterCount() +
1

[mvstanton, 2014-07-17 09:04:05]

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...
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...
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...
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#newcod...
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#newcod...
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#newcod...
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#newc...
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#newc...
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...
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.

[danno, 2014-07-17 09:27:32]

lgtm

lgtm

[danno, 2014-07-17 09:27:32]

lgtm

[mvstanton, 2014-07-17 11:50:32]

Committed patchset #8 manually as r22448.