Convert Linkage to use MachineSignature.

Convert Linkage to use MachineSignature.

This simplifies the handling of MachineTypes for parameters and returns
used in tests, and overall improves the regularity with which they
are handled in both tests and in CallDescriptor.

R=bmeurer@chromium.org, jarin@chromium.org
BUG=

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

[Benedikt Meurer, 2014-09-02 08:37:43]

First round of comments.

General note: Please use either parameter_count or param_count consistently.

https://codereview.chromium.org/530783002/diff/1/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/linkage.h#newco...
src/compiler/linkage.h:202: MachineType GetParameterType(int index) {
Nit: const

https://codereview.chromium.org/530783002/diff/1/src/compiler/linkage.h#newco...
src/compiler/linkage.h:212: MachineType GetReturnType() { return
incoming_->GetReturnType(0); }
Nit: const

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:116: Signature(uint8_t return_count, uint16_t
param_count, T* reps)
The Chrome Coding Style says: "Use unsigned integer types (preferably size_t)
for object and allocation sizes, object counts, array and pointer offsets,
vector indices, and so on. The signed types are incorrect and unsafe for these
purposes. (Integer overflow behavior for signed types is undefined in the C and
C++ standards, while the behavior is defined for unsigned types.) The C++ STL is
a guide here: they use size_t and foo::size_type for very good reasons.", and
esp. "Interfaces (function prototypes) for these uses should use size_t, even if
the underlying storage is a smaller unsigned type (e.g. as a space
optimization)."

So please use size_t for the interface, and DCHECK the range if you use uintX_t
for internal representation.

See http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:119: int ReturnCount() const { return return_count_;
}
size_t return_count() const { return return_count_; }

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:120: int ParamCount() const { return param_count_; }
size_t param_count() const { return param_count_; }

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:122: T GetParam(int index) const {
size_t for index

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:123: DCHECK(index >= 0 && index < param_count_);
DCHECK_LT(index, param_count());

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:124: return reps_[return_count_ + index];
reps_[return_count() + index]

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:127: T GetReturn(int index = 0) const {
size_t for index

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:128: DCHECK(index >= 0 && index < return_count_);
DCHECK_LT(index, return_count());

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:143: const int return_count_;
size_t

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:144: const int param_count_;
size_t

https://codereview.chromium.org/530783002/diff/1/src/compiler/raw-machine-ass...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/raw-machine-ass...
src/compiler/raw-machine-assembler.h:57: int parameter_count() { return
machine_sig_->ParamCount(); }
Nit: size_t + const

https://codereview.chromium.org/530783002/diff/1/src/compiler/structured-mach...
File src/compiler/structured-machine-assembler.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/structured-mach...
src/compiler/structured-machine-assembler.h:73: int parameter_count() { return
machine_sig_->ParamCount(); }
Nit: size_t + const

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
File test/cctest/compiler/call-tester.h (right):

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:144: int param_count = 5;
Nit: size_t

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:146: for (int i = param_count - 1; i >= 0;
i--) {
for (; param_count != 0 && types[param_count - 1] == kMachNone; --param_count) 
  ;

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:149: int return_count = return_type ==
kMachNone ? 0 : 1;
size_t

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:154: for (int i = 0; i < param_count; i++) {
size_t

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:162: void VerifyParameters(int
parameter_count, MachineType* parameter_types) {
size_t

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:164: for (int i = 0; i < parameter_count;
i++) {
size_t

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/graph-b...
File test/cctest/compiler/graph-builder-tester.h (right):

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/graph-b...
test/cctest/compiler/graph-builder-tester.h:51: int parameter_count() const {
return machine_sig_->ParamCount(); }
size_t

[titzer, 2014-09-02 12:13:53]

https://codereview.chromium.org/530783002/diff/1/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/linkage.h#newco...
src/compiler/linkage.h:202: MachineType GetParameterType(int index) {
On 2014/09/02 08:37:41, Benedikt Meurer wrote:
> Nit: const

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/linkage.h#newco...
src/compiler/linkage.h:212: MachineType GetReturnType() { return
incoming_->GetReturnType(0); }
On 2014/09/02 08:37:41, Benedikt Meurer wrote:
> Nit: const

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:116: Signature(uint8_t return_count, uint16_t
param_count, T* reps)
On 2014/09/02 08:37:41, Benedikt Meurer wrote:
> The Chrome Coding Style says: "Use unsigned integer types (preferably size_t)
> for object and allocation sizes, object counts, array and pointer offsets,
> vector indices, and so on. The signed types are incorrect and unsafe for these
> purposes. (Integer overflow behavior for signed types is undefined in the C
and
> C++ standards, while the behavior is defined for unsigned types.) The C++ STL
is
> a guide here: they use size_t and foo::size_type for very good reasons.", and
> esp. "Interfaces (function prototypes) for these uses should use size_t, even
if
> the underlying storage is a smaller unsigned type (e.g. as a space
> optimization)."
> 
> So please use size_t for the interface, and DCHECK the range if you use
uintX_t
> for internal representation.
> 
> See http://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:119: int ReturnCount() const { return return_count_;
}
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> size_t return_count() const { return return_count_; }

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:120: int ParamCount() const { return param_count_; }
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> size_t param_count() const { return param_count_; }

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:122: T GetParam(int index) const {
On 2014/09/02 08:37:41, Benedikt Meurer wrote:
> size_t for index

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:123: DCHECK(index >= 0 && index < param_count_);
On 2014/09/02 08:37:41, Benedikt Meurer wrote:
> DCHECK_LT(index, param_count());

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:124: return reps_[return_count_ + index];
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> reps_[return_count() + index]

This is mostly personal preference, but inside the class it's fine to use the
private members directly.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:127: T GetReturn(int index = 0) const {
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> size_t for index

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:128: DCHECK(index >= 0 && index < return_count_);
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> DCHECK_LT(index, return_count());

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:143: const int return_count_;
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> size_t

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/machine-type.h#...
src/compiler/machine-type.h:144: const int param_count_;
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> size_t

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/raw-machine-ass...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/raw-machine-ass...
src/compiler/raw-machine-assembler.h:57: int parameter_count() { return
machine_sig_->ParamCount(); }
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> Nit: size_t + const

Done.

https://codereview.chromium.org/530783002/diff/1/src/compiler/structured-mach...
File src/compiler/structured-machine-assembler.h (right):

https://codereview.chromium.org/530783002/diff/1/src/compiler/structured-mach...
src/compiler/structured-machine-assembler.h:73: int parameter_count() { return
machine_sig_->ParamCount(); }
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> Nit: size_t + const

Done.

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
File test/cctest/compiler/call-tester.h (right):

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:144: int param_count = 5;
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> Nit: size_t

Done.

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:146: for (int i = param_count - 1; i >= 0;
i--) {
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> for (; param_count != 0 && types[param_count - 1] == kMachNone; --param_count)

>   ;

Done.

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:149: int return_count = return_type ==
kMachNone ? 0 : 1;
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> size_t

Done.

https://codereview.chromium.org/530783002/diff/1/test/cctest/compiler/call-te...
test/cctest/compiler/call-tester.h:154: for (int i = 0; i < param_count; i++) {
On 2014/09/02 08:37:42, Benedikt Meurer wrote:
> size_t

Done.

[Benedikt Meurer, 2014-09-02 19:00:34]

LGTM with nits.

https://codereview.chromium.org/530783002/diff/80001/src/compiler/arm/linkage...
File src/compiler/arm/linkage-arm.cc (right):

https://codereview.chromium.org/530783002/diff/80001/src/compiler/arm/linkage...
src/compiler/arm/linkage-arm.cc:17: struct ArmLinkageHelperTraits {
I think this struct and the typedef below should be in an anonymous namespace.
Then you don't need to prefix with architecture name. (Same for other
linkage-arch files).

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage-impl.h
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage-imp...
src/compiler/linkage-impl.h:29: int js_parameter_count) {
size_t

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage-imp...
src/compiler/linkage-impl.h:44: for (int i = 0; i < js_parameter_count; i++) {
size_t

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage-imp...
src/compiler/linkage-impl.h:71: Zone* zone, Runtime::FunctionId function_id, int
js_parameter_count,
size_t

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage.h#n...
src/compiler/linkage.h:68: js_param_count_(js_param_count),
Nit: Rename to js_parameter_count_ for consistency.

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage.h#n...
src/compiler/linkage.h:109: MachineSignature* GetMachineSignature() const {
return machine_sig_; }
Nit: const MachineSignature*

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage.h#n...
src/compiler/linkage.h:136: size_t js_param_count_;
Nit: js_parameter_count_

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage.h#n...
src/compiler/linkage.h:202: MachineType GetParameterType(int index) {
Nit: method should be const

https://codereview.chromium.org/530783002/diff/80001/src/compiler/linkage.h#n...
src/compiler/linkage.h:212: MachineType GetReturnType() { return
incoming_->GetReturnType(0); }
Nit: method should be const

https://codereview.chromium.org/530783002/diff/80001/src/compiler/x64/linkage...
File src/compiler/x64/linkage-x64.cc (right):

https://codereview.chromium.org/530783002/diff/80001/src/compiler/x64/linkage...
src/compiler/x64/linkage-x64.cc:38: static Register CRegisterParameter(int i) {
This parameter should be size_t everywhere.

(Did I mention that I really hate this Linkage templatization?)

https://codereview.chromium.org/530783002/diff/80001/src/compiler/x64/linkage...
src/compiler/x64/linkage-x64.cc:47: static int CRegisterParametersLength() {
return kWin64 ? 4 : 6; }
This should return size_t as well.

[titzer, 2014-09-03 10:13:47]

Committed patchset #7 manually as 23638 (presubmit successful).