Eliminate stack adjustment for float-returning functions

Eliminate stack adjustment for float-returning functions

This involves changing AdjustStack to grow/shrink the stack, and to use that
operation exclusively to move the StackAdjustment variable in lowering, rather
than in call emission as before.

BUG=
R=stichnot@chromium.org

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

[sehr, 2015-11-13 21:13:42]

Description was changed from

==========
Eliminate stack adjustment for float-returning functions

This involves changing AdjustStack to grow/shrink the stack, and to use that
operation exclusively to move the StackAdjustment variable in lowering, rather
than in call emission as before.

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

to

==========
Eliminate stack adjustment for float-returning functions

This involves changing AdjustStack to grow/shrink the stack, and to use that
operation exclusively to move the StackAdjustment variable in lowering, rather
than in call emission as before.

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

[sehr, 2015-11-13 21:13:42]

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

[sehr, 2015-11-13 21:14:44]

Eliminates a sub/add pair.  PTAL.

[Jim Stichnoth, 2015-11-13 23:30:33]

A few small things, but otherwise LGTM.

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86Base.h#newcod...
src/IceInstX86Base.h:408: /// AdjustStack instruction - grows the stack (moves
esp down) by the given
Optional.  I like your idea of breaking this into two pseudo-instructions, e.g.
GrowStack and ContractStack, which abstracts away the built-in assumption of
which direction the stack grows.

You could probably do this through minor template wizardry:

  template <class Machine>
  template <bool Grow>
  class InstX86AdjustStack ... {
    const uint32_t Amount;
    constexpr bool IsGrow = Grow;
  };

  ...
  using GrowStack = InstX86AdjustStack<Machine><true>;
  using ContractStack = InstX86AdjustStack<Machine><false>;

(Completely untested, but hopefully illustrative.)

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86Base.h#newcod...
src/IceInstX86Base.h:432: int32_t Amount;
Oops, this should be const.

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86BaseImpl.h#ne...
src/IceInstX86BaseImpl.h:2642: // Hack this by caller-reserved memory at the top
of stack, spilling st(0)
by using

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5201: // Add a fake use of esp to make sure
esp stays alive for the entire
Could you factor this code and its lowerRet clone into a private helper
function?  Call it something like keepEspLiveAtExit().

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5203: // TODO: Are there more places where
the fake use should be inserted? E.g.
Remove this TODO because I'm pretty sure this covers everything by now.  The
non-returning test case seems to be fine.

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5206:
Func->getTarget()->getPhysicalRegister(Traits::RegisterSet::Reg_esp);
Use getStackReg() instead of Reg_esp.

BTW, getStackReg() will probably need to be put into Traits and redirected to
Reg_rsp under x86-64, but at least that's one fewer place to track down.

[sehr, 2015-11-14 00:30:56]

Thanks for the review.  Committing.

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86Base.h#newcod...
src/IceInstX86Base.h:408: /// AdjustStack instruction - grows the stack (moves
esp down) by the given
On 2015/11/13 23:30:33, stichnot wrote:
> Optional.  I like your idea of breaking this into two pseudo-instructions,
e.g.
> GrowStack and ContractStack, which abstracts away the built-in assumption of
> which direction the stack grows.
> 
> You could probably do this through minor template wizardry:
> 
>   template <class Machine>
>   template <bool Grow>
>   class InstX86AdjustStack ... {
>     const uint32_t Amount;
>     constexpr bool IsGrow = Grow;
>   };
> 
>   ...
>   using GrowStack = InstX86AdjustStack<Machine><true>;
>   using ContractStack = InstX86AdjustStack<Machine><false>;
> 
> (Completely untested, but hopefully illustrative.)

I like the idea too (obviously), but can't make the template stuff work at the
moment.  I prefer to punt this from this CL.

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86Base.h#newcod...
src/IceInstX86Base.h:432: int32_t Amount;
On 2015/11/13 23:30:33, stichnot wrote:
> Oops, this should be const.

Done.

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1449523002/diff/1/src/IceInstX86BaseImpl.h#ne...
src/IceInstX86BaseImpl.h:2642: // Hack this by caller-reserved memory at the top
of stack, spilling st(0)
On 2015/11/13 23:30:33, stichnot wrote:
> by using

Done.

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5201: // Add a fake use of esp to make sure
esp stays alive for the entire
On 2015/11/13 23:30:33, stichnot wrote:
> Could you factor this code and its lowerRet clone into a private helper
> function?  Call it something like keepEspLiveAtExit().

Done.

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5203: // TODO: Are there more places where
the fake use should be inserted? E.g.
On 2015/11/13 23:30:33, stichnot wrote:
> Remove this TODO because I'm pretty sure this covers everything by now.  The
> non-returning test case seems to be fine.

Done.

https://codereview.chromium.org/1449523002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5206:
Func->getTarget()->getPhysicalRegister(Traits::RegisterSet::Reg_esp);
On 2015/11/13 23:30:33, stichnot wrote:
> Use getStackReg() instead of Reg_esp.
> 
> BTW, getStackReg() will probably need to be put into Traits and redirected to
> Reg_rsp under x86-64, but at least that's one fewer place to track down.

Done.

[sehr, 2015-11-14 00:32:42]

Committed patchset #2 (id:20001) manually as
21fd103c847f7968e8df731d7bf4fa5733bd0f3f (presubmit successful).