[turbofan] CodeGenerator: Frame setup refactoring

[turbofan] CodeGenerator: Frame setup refactoring

Before frame elision, we finalized the frame shape when assembling the
prologue, which is also when we prepared the frame (saving sp, etc).

The frame finalization only needs to happen once, and happens to be
actually a set of idempotent operations. With frame elision, the logic for
frame finalization was happening every time we constructed the frame.
Albeit idempotent operations, the code would become hard to maintain.

This change separates frame shape finalization from frame
construction. When constructing the CodeGenerator, we finalize the
frame. Subsequent access is to a const Frame*.

Also renamed AssemblePrologue to AssembleConstructFrame, as
suggested in the frame elision CR.

Separating frame setup gave the opportunity to do away with
architecture-independent frame aligning (which is something just arm64
cares about), and also with stack pointer setup (also arm64). Both of
these happen now at frame finalization on arm64.

BUG=

Committed: https://crrev.com/81a1530e6fb3f4382098f71c01ffe047ffd72afc
Cr-Commit-Position: refs/heads/master@{#35642}

[Mircea Trofin, 2016-03-31 02:23:41]

Description was changed from

==========
Frame refactoring x64

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

to

==========
[turbofan] CodeGenerator: Frame setup refactoring

Before frame elision, we finalized the frame shape when assembling the
prologue, which is also when we prepared the frame (saving sp, etc).

The frame finalization only needs to happen once, and happens to be 
actually a set of idempotent operations. With frame elision, the logic for
frame finalization was happening every time we constructed the frame.
Albeit idempotent operations, the code would become hard to maintain.

This change separates frame shape finalization from frame 
construction. When constructing the CodeGenerator, we finalize the 
frame. Subsequent access is to a const Frame*.

Also renamed AssemblePrologue to AssembleConstructFrame, as 
suggested in the frame elision CR.

Separating frame setup gave the opportunity to do away with 
architecture-independent frame aligning (which is something just arm64
cares about), and also with stack pointer setup (also arm64). Both of 
these happen now at frame finalization on arm64.

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

[Mircea Trofin, 2016-03-31 02:23:41]

mtrofin@chromium.org changed reviewers:
+ bmeurer@chromium.org, danno@chromium.org, jarin@chromium.org

[Mircea Trofin, 2016-04-01 05:32:41]

On 2016/03/31 02:52:59, Mircea Trofin wrote:

ping :)

[danno, 2016-04-01 15:56:26]

first round of feedback

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/code-gener...
File src/compiler/code-generator.h (right):

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/code-gener...
src/compiler/code-generator.h:73: frame_access_state_ = new (code()->zone())
FrameAccessState(frame);
The body of this shouldn't be inline.

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h#ne...
src/compiler/frame.h:160: int stack_shrink_slots_;
I don't think this has to/should be separate from, say, spill_slot_count_. There
are three parts of the stack documented at the top of this class, the header,
spill slots and callee-saved. I think you want to keep just three areas, but
fold stack_shrink_slots_ actually can just get folded spill_slot_count, because
that's how OSR ends up handling these slots... just like spill slots (or it
should). Upshot: Please don't add anything to this class if you don't have to or
can model with something else that's already there.

In fact, I think as part of your cleanup, you probably want to get rid of
callee_saved_slot_count_, too, from here. You can model callee saved registers
better in the FrameAccessState. You can imagine a world where we only save
callee-saved register around the blocks that actually use them, and elide the
saves elsewhere, just like we do for frame elision. In FrameAccessState, you
just need to add a callee_saved_slot_count_ that gets added to 
callee_saved_slot_count_ at the right points. Then FrameAccessState, which is
mutable, can save/unsave callee-saved register whenever it wants.

[Mircea Trofin, 2016-04-01 18:27:39]

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h#ne...
src/compiler/frame.h:160: int stack_shrink_slots_;
On 2016/04/01 15:56:26, danno wrote:
> I don't think this has to/should be separate from, say, spill_slot_count_.
There
> are three parts of the stack documented at the top of this class, the header,
> spill slots and callee-saved. I think you want to keep just three areas, but
> fold stack_shrink_slots_ actually can just get folded spill_slot_count,
because
> that's how OSR ends up handling these slots... just like spill slots (or it
> should). Upshot: Please don't add anything to this class if you don't have to
or
> can model with something else that's already there.
> 
> In fact, I think as part of your cleanup, you probably want to get rid of
> callee_saved_slot_count_, too, from here. You can model callee saved registers
> better in the FrameAccessState. You can imagine a world where we only save
> callee-saved register around the blocks that actually use them, and elide the
> saves elsewhere, just like we do for frame elision. In FrameAccessState, you
> just need to add a callee_saved_slot_count_ that gets added to 
> callee_saved_slot_count_ at the right points. Then FrameAccessState, which is
> mutable, can save/unsave callee-saved register whenever it wants.

Do we even have a reason to handle callee saved registers as part of the frame
altogether? Here's an alternative: in the instruction sequence, parameters
passed by register appear as constraints after archNop instructions. 
[eax] = archNop
(v1 = eax)
...

We add constraints before the ret instruction to re-assign back to the incoming
register the virtual register value, and then let the register allocator handle
spilling. The concept of "callee saved registers" goes away from frames and code
generation:

()(eax = v1)
archRet

Thoughts?

[Benedikt Meurer, 2016-04-04 03:50:30]

On 2016/04/01 18:27:39, Mircea Trofin (OOO) wrote:
> https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h
> File src/compiler/frame.h (right):
> 
>
https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h#ne...
> src/compiler/frame.h:160: int stack_shrink_slots_;
> On 2016/04/01 15:56:26, danno wrote:
> > I don't think this has to/should be separate from, say, spill_slot_count_.
> There
> > are three parts of the stack documented at the top of this class, the
header,
> > spill slots and callee-saved. I think you want to keep just three areas, but
> > fold stack_shrink_slots_ actually can just get folded spill_slot_count,
> because
> > that's how OSR ends up handling these slots... just like spill slots (or it
> > should). Upshot: Please don't add anything to this class if you don't have
to
> or
> > can model with something else that's already there.
> > 
> > In fact, I think as part of your cleanup, you probably want to get rid of
> > callee_saved_slot_count_, too, from here. You can model callee saved
registers
> > better in the FrameAccessState. You can imagine a world where we only save
> > callee-saved register around the blocks that actually use them, and elide
the
> > saves elsewhere, just like we do for frame elision. In FrameAccessState, you
> > just need to add a callee_saved_slot_count_ that gets added to 
> > callee_saved_slot_count_ at the right points. Then FrameAccessState, which
is
> > mutable, can save/unsave callee-saved register whenever it wants.
> 
> Do we even have a reason to handle callee saved registers as part of the frame
> altogether? Here's an alternative: in the instruction sequence, parameters
> passed by register appear as constraints after archNop instructions. 
> [eax] = archNop
> (v1 = eax)
> ...
> 
> We add constraints before the ret instruction to re-assign back to the
incoming
> register the virtual register value, and then let the register allocator
handle
> spilling. The concept of "callee saved registers" goes away from frames and
code
> generation:
> 
> ()(eax = v1)
> archRet
> 
> Thoughts?

I like this idea. But I'm not sure how well we can integrate this into the
InstructionSelector. Last time we tried to add artificial live ranges, that
didn't look too readable. Do you have a prototype?

[Mircea Trofin, 2016-04-15 22:50:19]

Patchset #8 (id:130001) has been deleted

[Mircea Trofin, 2016-04-16 04:25:10]

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/1843143002/diff/90001/src/compiler/frame.h#ne...
src/compiler/frame.h:160: int stack_shrink_slots_;
On 2016/04/01 15:56:26, danno wrote:
> I don't think this has to/should be separate from, say, spill_slot_count_.
There
> are three parts of the stack documented at the top of this class, the header,
> spill slots and callee-saved. I think you want to keep just three areas, but
> fold stack_shrink_slots_ actually can just get folded spill_slot_count,
because
> that's how OSR ends up handling these slots... just like spill slots (or it
> should). Upshot: Please don't add anything to this class if you don't have to
or
> can model with something else that's already there.
> 
> In fact, I think as part of your cleanup, you probably want to get rid of
> callee_saved_slot_count_, too, from here. You can model callee saved registers
> better in the FrameAccessState. You can imagine a world where we only save
> callee-saved register around the blocks that actually use them, and elide the
> saves elsewhere, just like we do for frame elision. In FrameAccessState, you
> just need to add a callee_saved_slot_count_ that gets added to 
> callee_saved_slot_count_ at the right points. Then FrameAccessState, which is
> mutable, can save/unsave callee-saved register whenever it wants.

Done, except with the moving of the callee saved registers stuff to
FrameAccessState. I'd prefer doing that
when/if we add the logic to selectively spill them, rather than whole sale
spilling them, as we do today.

https://codereview.chromium.org/1843143002/diff/190001/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/1843143002/diff/190001/src/compiler/frame.h#n...
src/compiler/frame.h:107: spill_slot_count_ += delta;
Note to reviewers: I am not very happy with this. On one hand, aligning here
does change the layout of the
frame, and in a sense, it does this by adding unused spill slots. On the other
hand, I would prefer the 
number of actual spill slots be remembered. I'm concerned with potential access
by the runtime into these
padding slots.

Thoughts?

[Mircea Trofin, 2016-04-16 15:16:21]

Patchset #11 (id:210001) has been deleted

[Mircea Trofin, 2016-04-16 15:25:44]

https://codereview.chromium.org/1843143002/diff/190001/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/1843143002/diff/190001/src/compiler/frame.h#n...
src/compiler/frame.h:107: spill_slot_count_ += delta;
On 2016/04/16 04:25:10, Mircea Trofin wrote:
> Note to reviewers: I am not very happy with this. On one hand, aligning here
> does change the layout of the
> frame, and in a sense, it does this by adding unused spill slots. On the other
> hand, I would prefer the 
> number of actual spill slots be remembered. I'm concerned with potential
access
> by the runtime into these
> padding slots.
> 
> Thoughts?

FWIW, I see we do the same in AlignFrame. Still, is there any merit to my
concern?

[Mircea Trofin, 2016-04-20 05:01:31]

On 2016/04/16 15:25:44, Mircea Trofin wrote:
> https://codereview.chromium.org/1843143002/diff/190001/src/compiler/frame.h
> File src/compiler/frame.h (right):
> 
>
https://codereview.chromium.org/1843143002/diff/190001/src/compiler/frame.h#n...
> src/compiler/frame.h:107: spill_slot_count_ += delta;
> On 2016/04/16 04:25:10, Mircea Trofin wrote:
> > Note to reviewers: I am not very happy with this. On one hand, aligning here
> > does change the layout of the
> > frame, and in a sense, it does this by adding unused spill slots. On the
other
> > hand, I would prefer the 
> > number of actual spill slots be remembered. I'm concerned with potential
> access
> > by the runtime into these
> > padding slots.
> > 
> > Thoughts?
> 
> FWIW, I see we do the same in AlignFrame. Still, is there any merit to my
> concern?

Ping :)

[Benedikt Meurer, 2016-04-20 05:05:10]

LGTM.

[Mircea Trofin, 2016-04-20 05:13:47]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-04-20 05:13:56]

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

[commit-bot: I haz the power, 2016-04-20 05:40:24]

Description was changed from

==========
[turbofan] CodeGenerator: Frame setup refactoring

Before frame elision, we finalized the frame shape when assembling the
prologue, which is also when we prepared the frame (saving sp, etc).

The frame finalization only needs to happen once, and happens to be 
actually a set of idempotent operations. With frame elision, the logic for
frame finalization was happening every time we constructed the frame.
Albeit idempotent operations, the code would become hard to maintain.

This change separates frame shape finalization from frame 
construction. When constructing the CodeGenerator, we finalize the 
frame. Subsequent access is to a const Frame*.

Also renamed AssemblePrologue to AssembleConstructFrame, as 
suggested in the frame elision CR.

Separating frame setup gave the opportunity to do away with 
architecture-independent frame aligning (which is something just arm64
cares about), and also with stack pointer setup (also arm64). Both of 
these happen now at frame finalization on arm64.

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

to

==========
[turbofan] CodeGenerator: Frame setup refactoring

Before frame elision, we finalized the frame shape when assembling the
prologue, which is also when we prepared the frame (saving sp, etc).

The frame finalization only needs to happen once, and happens to be 
actually a set of idempotent operations. With frame elision, the logic for
frame finalization was happening every time we constructed the frame.
Albeit idempotent operations, the code would become hard to maintain.

This change separates frame shape finalization from frame 
construction. When constructing the CodeGenerator, we finalize the 
frame. Subsequent access is to a const Frame*.

Also renamed AssemblePrologue to AssembleConstructFrame, as 
suggested in the frame elision CR.

Separating frame setup gave the opportunity to do away with 
architecture-independent frame aligning (which is something just arm64
cares about), and also with stack pointer setup (also arm64). Both of 
these happen now at frame finalization on arm64.

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

[commit-bot: I haz the power, 2016-04-20 05:40:25]

Committed patchset #10 (id:190001)

[nodir1, 2016-04-22 18:43:38]

Description was changed from

==========
[turbofan] CodeGenerator: Frame setup refactoring

Before frame elision, we finalized the frame shape when assembling the
prologue, which is also when we prepared the frame (saving sp, etc).

The frame finalization only needs to happen once, and happens to be 
actually a set of idempotent operations. With frame elision, the logic for
frame finalization was happening every time we constructed the frame.
Albeit idempotent operations, the code would become hard to maintain.

This change separates frame shape finalization from frame 
construction. When constructing the CodeGenerator, we finalize the 
frame. Subsequent access is to a const Frame*.

Also renamed AssemblePrologue to AssembleConstructFrame, as 
suggested in the frame elision CR.

Separating frame setup gave the opportunity to do away with 
architecture-independent frame aligning (which is something just arm64
cares about), and also with stack pointer setup (also arm64). Both of 
these happen now at frame finalization on arm64.

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

to

==========
[turbofan] CodeGenerator: Frame setup refactoring

Before frame elision, we finalized the frame shape when assembling the
prologue, which is also when we prepared the frame (saving sp, etc).

The frame finalization only needs to happen once, and happens to be
actually a set of idempotent operations. With frame elision, the logic for
frame finalization was happening every time we constructed the frame.
Albeit idempotent operations, the code would become hard to maintain.

This change separates frame shape finalization from frame
construction. When constructing the CodeGenerator, we finalize the
frame. Subsequent access is to a const Frame*.

Also renamed AssemblePrologue to AssembleConstructFrame, as
suggested in the frame elision CR.

Separating frame setup gave the opportunity to do away with
architecture-independent frame aligning (which is something just arm64
cares about), and also with stack pointer setup (also arm64). Both of
these happen now at frame finalization on arm64.

BUG=

Committed: https://crrev.com/81a1530e6fb3f4382098f71c01ffe047ffd72afc
Cr-Commit-Position: refs/heads/master@{#35642}
==========

[nodir1, 2016-04-22 18:43:39]

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