[turbofan] Add general support for sp-based frame access

[turbofan] Add general support for sp-based frame access

Some highlights of this CL:
* Refactor the mutable state out of Frame into FrameAccessState,
  which is maintained and updated during code generation to
  record whether sp- or fp-based frame access is currently active
  and how deep the stack on top of the frame is.
* The operand resultion in linkage.cc now uses FrameAccessState
  to determine how to generate frame-accessing operands.
* Update all platforms to accurately track additionally pushed
  stack slots (e.g. arguments for calls) in the FrameAccessState.
* Add a flag, --turbo_sp_frame_access, which forces all frame
  access to be sp-based whenever possible. This will likely never
  be used in production, but for testing it's useful in verifying
  that the stack-tracking of each platform maintained in the
  FrameAccessState is correct.
* Use sp-based frame access for gap resolving before tail
  calls. This will allow for slightly more efficient restoration
  of the frame pointer in the tail call in a later CL.
* Remove most ad hoc groping into CallDescriptors to
  determine if a frame is needed, instead consistently use
  predicates like needs_frame(), IsCFunctionCall() and
  IsJSFunctionCall().

BUG=v8:4076
LOG=n

Committed: https://crrev.com/51669873694ed266ea06a2726f87138374eea1e7
Cr-Commit-Position: refs/heads/master@{#32234}

[danno, 2015-11-20 09:47:17]

Description was changed from

==========
[turbofan] Add general support for sp-based frame access

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

to

==========
[turbofan] Add general support for sp-based frame access

Some highlights of this CL:
* Refactor the mutable state out of Frame into FrameAccessState,
  which is maintained and updated during code generation to
  record whether sp- or fp-based frame access is currently active
  and how deep the stack on top of the frame is.
* The operand resultion in linkage.cc now uses FrameAccessState
  to determine how to generate frame-accessing operands.
* Update all platforms to accurately track additionally pushed
  stack slots (e.g. arguments for calls) in the FrameAccessState.
* Add a flag, --turbo_sp_frame_access, which forces all frame
  access to be sp-based whenever possible. This will likely never
  be used in production, but for testing it's useful in verifying
  that the stack-tracking of each platform maintained in the
  FrameAccessState is correct.
* Use sp-based frame access for gap resolving before tail
  calls. This will allow for slightly more efficient restoration
  of the frame pointer in the tail call in a later CL.

BUG=v8:4076
LOG=n
==========

[Benedikt Meurer, 2015-11-20 09:53:09]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-11-20 09:53:10]

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

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/linkage.h#...
src/compiler/linkage.h:357: FrameAccessState* frame_access_state) const;
Nit: pass as const* or const&.

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:277: descriptor->kind() ==
CallDescriptor::kCallAddress;
Nit: this looks somewhat ad-hoc; can we have some predicate here?

[danno, 2015-11-20 10:07:11]

danno@chromium.org changed reviewers:
+ mtrofin@chromium.org

[danno, 2015-11-20 10:07:11]

danno@chromium.org changed required reviewers:
+ bmeurer@chromium.org

[danno, 2015-11-23 14:34:47]

Description was changed from

==========
[turbofan] Add general support for sp-based frame access

Some highlights of this CL:
* Refactor the mutable state out of Frame into FrameAccessState,
  which is maintained and updated during code generation to
  record whether sp- or fp-based frame access is currently active
  and how deep the stack on top of the frame is.
* The operand resultion in linkage.cc now uses FrameAccessState
  to determine how to generate frame-accessing operands.
* Update all platforms to accurately track additionally pushed
  stack slots (e.g. arguments for calls) in the FrameAccessState.
* Add a flag, --turbo_sp_frame_access, which forces all frame
  access to be sp-based whenever possible. This will likely never
  be used in production, but for testing it's useful in verifying
  that the stack-tracking of each platform maintained in the
  FrameAccessState is correct.
* Use sp-based frame access for gap resolving before tail
  calls. This will allow for slightly more efficient restoration
  of the frame pointer in the tail call in a later CL.

BUG=v8:4076
LOG=n
==========

to

==========
[turbofan] Add general support for sp-based frame access

Some highlights of this CL:
* Refactor the mutable state out of Frame into FrameAccessState,
  which is maintained and updated during code generation to
  record whether sp- or fp-based frame access is currently active
  and how deep the stack on top of the frame is.
* The operand resultion in linkage.cc now uses FrameAccessState
  to determine how to generate frame-accessing operands.
* Update all platforms to accurately track additionally pushed
  stack slots (e.g. arguments for calls) in the FrameAccessState.
* Add a flag, --turbo_sp_frame_access, which forces all frame
  access to be sp-based whenever possible. This will likely never
  be used in production, but for testing it's useful in verifying
  that the stack-tracking of each platform maintained in the
  FrameAccessState is correct.
* Use sp-based frame access for gap resolving before tail
  calls. This will allow for slightly more efficient restoration
  of the frame pointer in the tail call in a later CL.
* Remove must ad hoc groping into CallDescriptors to
  determine if a frame is needed, instead consistently use
  predicates like needs_frame(), IsCFunctionCall() and
  IsJSFunctionCall().

BUG=v8:4076
LOG=n
==========

[danno, 2015-11-23 14:35:01]

Description was changed from

==========
[turbofan] Add general support for sp-based frame access

Some highlights of this CL:
* Refactor the mutable state out of Frame into FrameAccessState,
  which is maintained and updated during code generation to
  record whether sp- or fp-based frame access is currently active
  and how deep the stack on top of the frame is.
* The operand resultion in linkage.cc now uses FrameAccessState
  to determine how to generate frame-accessing operands.
* Update all platforms to accurately track additionally pushed
  stack slots (e.g. arguments for calls) in the FrameAccessState.
* Add a flag, --turbo_sp_frame_access, which forces all frame
  access to be sp-based whenever possible. This will likely never
  be used in production, but for testing it's useful in verifying
  that the stack-tracking of each platform maintained in the
  FrameAccessState is correct.
* Use sp-based frame access for gap resolving before tail
  calls. This will allow for slightly more efficient restoration
  of the frame pointer in the tail call in a later CL.
* Remove must ad hoc groping into CallDescriptors to
  determine if a frame is needed, instead consistently use
  predicates like needs_frame(), IsCFunctionCall() and
  IsJSFunctionCall().

BUG=v8:4076
LOG=n
==========

to

==========
[turbofan] Add general support for sp-based frame access

Some highlights of this CL:
* Refactor the mutable state out of Frame into FrameAccessState,
  which is maintained and updated during code generation to
  record whether sp- or fp-based frame access is currently active
  and how deep the stack on top of the frame is.
* The operand resultion in linkage.cc now uses FrameAccessState
  to determine how to generate frame-accessing operands.
* Update all platforms to accurately track additionally pushed
  stack slots (e.g. arguments for calls) in the FrameAccessState.
* Add a flag, --turbo_sp_frame_access, which forces all frame
  access to be sp-based whenever possible. This will likely never
  be used in production, but for testing it's useful in verifying
  that the stack-tracking of each platform maintained in the
  FrameAccessState is correct.
* Use sp-based frame access for gap resolving before tail
  calls. This will allow for slightly more efficient restoration
  of the frame pointer in the tail call in a later CL.
* Remove most ad hoc groping into CallDescriptors to
  determine if a frame is needed, instead consistently use
  predicates like needs_frame(), IsCFunctionCall() and
  IsJSFunctionCall().

BUG=v8:4076
LOG=n
==========

[danno, 2015-11-23 14:35:22]

Feedback addressed, PTAL

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:277: descriptor->kind() ==
CallDescriptor::kCallAddress;
On 2015/11/20 09:53:10, Benedikt Meurer wrote:
> Nit: this looks somewhat ad-hoc; can we have some predicate here?

Done.

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:277: descriptor->kind() ==
CallDescriptor::kCallAddress;
On 2015/11/20 09:53:10, Benedikt Meurer wrote:
> Nit: this looks somewhat ad-hoc; can we have some predicate here?

Done.

[danno, 2015-11-23 14:36:11]

danno@chromium.org changed reviewers:
+ jarin@chromium.org

[Mircea Trofin, 2015-11-23 16:41:57]

On 2015/11/23 14:35:22, danno wrote:
> Feedback addressed, PTAL
> 
> https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.cc
> File src/compiler/pipeline.cc (right):
> 
>
https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.c...
> src/compiler/pipeline.cc:277: descriptor->kind() ==
> CallDescriptor::kCallAddress;
> On 2015/11/20 09:53:10, Benedikt Meurer wrote:
> > Nit: this looks somewhat ad-hoc; can we have some predicate here?
> 
> Done.
> 
>
https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.c...
> src/compiler/pipeline.cc:277: descriptor->kind() ==
> CallDescriptor::kCallAddress;
> On 2015/11/20 09:53:10, Benedikt Meurer wrote:
> > Nit: this looks somewhat ad-hoc; can we have some predicate here?
> 
> Done.

should we have some unit testing for this?
should we add a benchmark that will benefit from this CL, once tail calls are
enabled?

[titzer, 2015-11-23 20:00:53]

titzer@chromium.org changed reviewers:
+ titzer@chromium.org

[titzer, 2015-11-23 20:00:54]

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/code-gene...
File src/compiler/code-generator.h (right):

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/code-gene...
src/compiler/code-generator.h:8: #include "src/compiler/frame-access-state.h"
You don't need this include if you forward declare the struct as you do, below.

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/frame-acc...
File src/compiler/frame-access-state.h (right):

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/frame-acc...
src/compiler/frame-access-state.h:16: class FrameAccessState : public ZoneObject
{
What about moving this class into frame.h? The two are so tightly coupled that
they almost always would be used together, I guess.

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/frame-acc...
src/compiler/frame-access-state.h:31: void UseFPToAccessFrame() {
access_frame_with_fp_ = true; }
The name of these methods doesn't really indicate that they alter state. Maybe
SetFrameAccessToFP(bool)?

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/linkage.c...
src/compiler/linkage.cc:151: FrameOffset Linkage::GetFrameOffset(
Now that I think about this method in modern times, it strikes me that it's a
little misplaced. It doesn't seem to access anything from the linkage itself, so
maybe it should be moved?

[danno, 2015-11-24 12:02:00]

Feedback addressed. 

Apropos testing: the purpose of the turbo_sp_frame_access flag is to allow the
sp-based frame access to be tested with the broadest possible coverage on a
dedicated bot. I am not sure what kind of unit tests could materially improve
over that kind of testing, especially since they would likely need to be
platform-specific to get sufficient coverage. I argue that the flag is enough.

As far as a benchmark is concerned, I don't think we necessarily need one. We'll
need sp-based frame access for frame elision in general, and this CL is as much
about the cleanup to make that happen in a readable and maintainable way as much
as it is about squeezing that last bit of performance out tail calls.

Furthermore, since we currently don't support tail calls in "real JavaScript",
an artificial benchmark would have to be constructed by hand, and I'm loath to
do that now. However, the interpreter's handlers are all written using tail
calls, and when we start optimizing those, including adding runtime-calling slow
paths, we may be able to measure the impact. Let's just wait for that.

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

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/linkage.h#...
src/compiler/linkage.h:357: FrameAccessState* frame_access_state) const;
On 2015/11/20 09:53:10, Benedikt Meurer wrote:
> Nit: pass as const* or const&.

Done.

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1460183002/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:277: descriptor->kind() ==
CallDescriptor::kCallAddress;
On 2015/11/20 09:53:10, Benedikt Meurer wrote:
> Nit: this looks somewhat ad-hoc; can we have some predicate here?

Done.

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/code-gene...
File src/compiler/code-generator.h (right):

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/code-gene...
src/compiler/code-generator.h:8: #include "src/compiler/frame-access-state.h"
On 2015/11/23 20:00:54, titzer wrote:
> You don't need this include if you forward declare the struct as you do,
below.

Done.

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/frame-acc...
File src/compiler/frame-access-state.h (right):

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/frame-acc...
src/compiler/frame-access-state.h:16: class FrameAccessState : public ZoneObject
{
On 2015/11/23 20:00:54, titzer wrote:
> What about moving this class into frame.h? The two are so tightly coupled that
> they almost always would be used together, I guess.

Done.

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/frame-acc...
src/compiler/frame-access-state.h:31: void UseFPToAccessFrame() {
access_frame_with_fp_ = true; }
On 2015/11/23 20:00:54, titzer wrote:
> The name of these methods doesn't really indicate that they alter state. Maybe
> SetFrameAccessToFP(bool)?

Done.

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1460183002/diff/140001/src/compiler/linkage.c...
src/compiler/linkage.cc:151: FrameOffset Linkage::GetFrameOffset(
On 2015/11/23 20:00:54, titzer wrote:
> Now that I think about this method in modern times, it strikes me that it's a
> little misplaced. It doesn't seem to access anything from the linkage itself,
so
> maybe it should be moved?

Done.

[Benedikt Meurer, 2015-11-24 15:37:07]

I think it's fine wrt. testing and benchmarks. The interpreter and code stubs
should start using this soonish. I like the dedicated bot idea.

LGTM from my side.

[Mircea Trofin, 2015-11-24 16:18:09]

On 2015/11/24 15:37:07, Benedikt Meurer wrote:
> I think it's fine wrt. testing and benchmarks. The interpreter and code stubs
> should start using this soonish. I like the dedicated bot idea.
> 
> LGTM from my side.

Thanks for the clarification wrt testing + benchmarking - lgtm, too

[titzer, 2015-11-24 17:45:57]

LGTM too

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

https://codereview.chromium.org/1460183002/diff/160001/src/compiler/linkage.h...
src/compiler/linkage.h:26: class FrameAccessState;
I think you can drop this forward declaration now.

[danno, 2015-11-24 17:46:37]

The CQ bit was checked by danno@chromium.org

[commit-bot: I haz the power, 2015-11-24 17:46:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460183002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460183002/160001

[danno, 2015-11-24 17:46:59]

The CQ bit was unchecked by danno@chromium.org

[danno, 2015-11-24 21:39:36]

The CQ bit was checked by danno@chromium.org

[danno, 2015-11-24 21:39:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, titzer@chromium.org, mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/1460183002/#ps180001
(title: "Last comment addressed")

[commit-bot: I haz the power, 2015-11-24 21:39:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1460183002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1460183002/180001

[commit-bot: I haz the power, 2015-11-24 22:53:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-24 22:54:02]

Patchset 10 (id:??) landed as
https://crrev.com/51669873694ed266ea06a2726f87138374eea1e7
Cr-Commit-Position: refs/heads/master@{#32234}