Merge lowerCall and lowerRet between x86 and x64

Merge lowerCall and lowerRet between x86 and x64

BUG=
R=jpp@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=0c68bef895ea8987922922ede6f1918763eaa00b

[sehr, 2016-01-19 19:14:38]

Description was changed from

==========
Merge lowerCall and lowerRet between x86 and x64

BUG=
==========

to

==========
Merge lowerCall and lowerRet between x86 and x64

BUG=
==========

[sehr, 2016-01-19 19:14:38]

sehr@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org

[sehr, 2016-01-19 19:15:14]

I may add the addProlog/addEpilog stuff to this CL, but I may also get
interrupted over the next few days, so I submit this for review now.

[Jim Stichnoth, 2016-01-19 20:54:06]

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:679: static const bool
X86_PASS_SCALAR_FP_IN_XMM = false;
Can this be constexpr?

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:681: static inline int32_t
getRegisterForXmmArgNum(uint32_t ArgNum) {
Remove "inline" for consistency?

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:685: return
static_cast<int32_t>(RegisterSet::Reg_xmm0 + ArgNum);
This would break if we for some reason decided to list the xmm registers in a
different order, i.e. not starting from xmm0.

I would leave a static_assert that xmm0+1==xmm1, plus a TODO to deal with this
the same way it was done for ARM.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:290: const Type ReturnType) = 0;
I find it a bit odd/unusual to declare a scalar parameter as "const".

(here and elsewhere)

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2120: int32_t ParameterAreaSizeBytes = 0;
Make this uint32_t instead.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2122: // Classify each argument operand
according to the location where the
reflow to 80-col

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2126: Type Ty = Arg->getType();
const

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2154: Variable *Dest = Instr->getDest();
Add something like:
  const Type DestTy = Dest ? Dest->getType() : IceType_void;
and use DestTy below as appropriate.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2165:
assert(static_cast<uint32_t>(ParameterAreaSizeBytes) <=
Remove this static_cast, assuming ParameterAreaSizeBytes can be declared as
uint32_t.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2169: for (SizeT i = 0, e = StackArgs.size();
i < e; ++i) {
Use NumStackArgs instead of "e", for consistency with above and below.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2173: // TODO: Investigate the impact of
lowering arguments passed in registers
I think this TODO should just be removed entirely.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2253: // Mark the call as killing all the
callee-saves registers.
caller-save

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2272: if (!Dest)
Dest == nullptr

[sehr, 2016-01-19 21:47:25]

Code review comments

[sehr, 2016-01-19 21:47:36]

PTAL.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:679: static const bool
X86_PASS_SCALAR_FP_IN_XMM = false;
On 2016/01/19 20:54:05, stichnot wrote:
> Can this be constexpr?

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:681: static inline int32_t
getRegisterForXmmArgNum(uint32_t ArgNum) {
On 2016/01/19 20:54:05, stichnot wrote:
> Remove "inline" for consistency?

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:685: return
static_cast<int32_t>(RegisterSet::Reg_xmm0 + ArgNum);
On 2016/01/19 20:54:05, stichnot wrote:
> This would break if we for some reason decided to list the xmm registers in a
> different order, i.e. not starting from xmm0.
> 
> I would leave a static_assert that xmm0+1==xmm1, plus a TODO to deal with this
> the same way it was done for ARM.

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:290: const Type ReturnType) = 0;
On 2016/01/19 20:54:05, stichnot wrote:
> I find it a bit odd/unusual to declare a scalar parameter as "const".
> 
> (here and elsewhere)

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2120: int32_t ParameterAreaSizeBytes = 0;
On 2016/01/19 20:54:06, stichnot wrote:
> Make this uint32_t instead.

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2122: // Classify each argument operand
according to the location where the
On 2016/01/19 20:54:05, stichnot wrote:
> reflow to 80-col

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2126: Type Ty = Arg->getType();
On 2016/01/19 20:54:06, stichnot wrote:
> const

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2154: Variable *Dest = Instr->getDest();
On 2016/01/19 20:54:06, stichnot wrote:
> Add something like:
>   const Type DestTy = Dest ? Dest->getType() : IceType_void;
> and use DestTy below as appropriate.

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2165:
assert(static_cast<uint32_t>(ParameterAreaSizeBytes) <=
On 2016/01/19 20:54:06, stichnot wrote:
> Remove this static_cast, assuming ParameterAreaSizeBytes can be declared as
> uint32_t.

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2169: for (SizeT i = 0, e = StackArgs.size();
i < e; ++i) {
On 2016/01/19 20:54:06, stichnot wrote:
> Use NumStackArgs instead of "e", for consistency with above and below.

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2173: // TODO: Investigate the impact of
lowering arguments passed in registers
On 2016/01/19 20:54:06, stichnot wrote:
> I think this TODO should just be removed entirely.

Done.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2253: // Mark the call as killing all the
callee-saves registers.
On 2016/01/19 20:54:06, stichnot wrote:
> caller-save

Duh.  Fixed.

https://codereview.chromium.org/1592033002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2272: if (!Dest)
On 2016/01/19 20:54:06, stichnot wrote:
> Dest == nullptr

Done.

[Jim Stichnoth, 2016-01-19 23:55:45]

lgtm

https://codereview.chromium.org/1592033002/diff/100001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1592033002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2245: // Mark the call as killing all the
caller-saves registers.
I would write "caller-save" for consistency with other places.

[John, 2016-01-20 00:04:34]

lgtm

https://codereview.chromium.org/1592033002/diff/100001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1592033002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2092: namespace {
Anonymous namespaces are dangerous in .h files. Maybe eliminate the namespace,
and Mark the function inline?

[sehr, 2016-01-20 16:07:10]

Thanks for the reviews.  Testing...

https://codereview.chromium.org/1592033002/diff/100001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1592033002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2092: namespace {
On 2016/01/20 00:04:34, John wrote:
> Anonymous namespaces are dangerous in .h files. Maybe eliminate the namespace,
> and Mark the function inline?

Done.

https://codereview.chromium.org/1592033002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2245: // Mark the call as killing all the
caller-saves registers.
On 2016/01/19 23:55:45, stichnot wrote:
> I would write "caller-save" for consistency with other places.

Done.

[sehr, 2016-01-20 18:00:28]

Description was changed from

==========
Merge lowerCall and lowerRet between x86 and x64

BUG=
==========

to

==========
Merge lowerCall and lowerRet between x86 and x64

BUG=
R=jpp@chromium.org, stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[sehr, 2016-01-20 18:00:29]

Committed patchset #6 (id:100001) manually as
0c68bef895ea8987922922ede6f1918763eaa00b (tree was closed).