[turbofan] Frame elision for code stubs

[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG=v8:4533
LOG=N

Committed: https://crrev.com/53d51c52f3754fe0e7decacba4277e58cf9fb5a4
Cr-Commit-Position: refs/heads/master@{#35139}

[Mircea Trofin, 2016-03-16 15:00:38]

Patchset #1 (id:1) has been deleted

[Mircea Trofin, 2016-03-16 15:01:08]

Description was changed from

==========
calculate frame location correctly when frame eliding

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

to

==========
Frame elision for code stubs

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

[danno, 2016-03-16 18:18:10]

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

[danno, 2016-03-16 18:18:11]

Looks like you're making progress.

I have some fundamental feedback about how the plumbing here works, please let
me know if you have questions.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:247: if
(!frame_access_state()->access_frame_with_fp()) {
Here and below, and on other ARM-like platforms, I don't think is is correct.
You can set up frame access with the SP even if you have a frame, this is what
happens in tail calls and there is a bot that builds and runs all of the tests
running with sp-based (not fp-based) frame access. You really need a predicate
on the frame_access_state() called has_frame().

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.cc:170: bool leaves_function =
This should not be calculated here.  When the blocks are built, or when the
frame elision analysis happens, must_decontruct_frame should be (re)set to false
for the block if the block ends in a deopt or return. That simplifies this logic
a lot, or at least puts the decision in a better place, and I think it makes the
decision much cleaner, you don't have to check every instruction, just the last
one. i.e. somewhere you need:

Instruction* instr = code()->InstructionAt(block->last_instruction_index())
bool leaves_function = instr-> .... as before
if (leaves_function) block->set_must_deconstruct_frame(false);

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

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.h:266: bool deconstruct_frame_when_leaving_;
I don't think you want this variable here, I think it's much better encapsulated
in the frame_access_state. I would much prefer a boolean on the
frame_access_state called currently_has_frame_. If starts out as false, and you
can query it. As you run through the blocks, you set it depending on
must_construct/deconstruct frame.

There would also be a method called SetCurrentlyHasFrame, it would look
something like this (maybe better in two methods, ConstructFrame and
DeconstructFrame with appropriate asserts.

SetCurrentlyHasFrame(bool has_frame) {
  has_frame_ = has_frame;
  if (has_frame) {
    SetFrameAccessToDefault();
  } else {
    SetFrameAccessToSP();
  }
}

GetSPToFPSlotCount would also be simpler, and for the most part remains
unchanged.

GetSPToFPSlotCount() {
 int used_slots = has_frame_ ? frame_->GetSPToFPSlotCount() :
(kElidedFrameSlotCount - StandardFrameConstants::kFixedSlotCountAboveFp);
 return used_slots + sp_delta()
}

Then, at the end of the method, you just query frame_access_state()->has_frame()
to know if you need to deconstruct the frame.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.h:267: bool deconstruct_frame_between_blocks_;
I don't think this boolean belongs here at all. See my comment in the .cc file
about pushing this into the block->must_deconstruct_frame() decision, and that
logic should be in the frame elision pass or in instruction selection or
wherever, but not here.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h
File src/compiler/frame.h (left):

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h#ol...
src/compiler/frame.h:89: inline int GetSPToFPSlotCount() const {
I don't think you should remove this, see below.

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

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h#ne...
src/compiler/frame.h:210: void set_used_frame_slots(int used_slots) {
used_frame_slots_ = used_slots; }
I don't think you need to track used_frame_slots explicitly if you have
has_frame_ as described elsewhere in my comments, I would prefer to have the
logic leak out as it kind of has in your CL.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h#ne...
src/compiler/frame.h:217: return used_frame_slots() -
StandardFrameConstants::kFixedSlotCountAboveFp +
I don't understand this change. The frame size frame_->GetSPToFPSlotCount()
should be constant and set up knowing the the maximum slot count for the frame
when it's present, regardless of whether there is a frame in this very moment or
not. 

So the implementation should make the decision something like this:

DCHECK(access_frame_with_fp());
int frame_slot_count =  has_frame_ ? frame_->GetSPToFPSlotCount() :
kElidedFrameSlotCount;
return frame_slot_count + sp_delta();

[Mircea Trofin, 2016-03-16 20:20:46]

Thanks for the feedback, added some (and a question or 2) of my own. I'm looking
at the OOL stuff in the meantime.

Thanks!

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:247: if
(!frame_access_state()->access_frame_with_fp()) {
On 2016/03/16 18:18:11, danno wrote:
> Here and below, and on other ARM-like platforms, I don't think is is correct.
> You can set up frame access with the SP even if you have a frame, this is what
> happens in tail calls and there is a bot that builds and runs all of the tests
> running with sp-based (not fp-based) frame access. You really need a predicate
> on the frame_access_state() called has_frame().

Ah, yes! I started from Intel and ended up cargo-culting the change from
frame()->needs_frame() over. In  my old prototype, I had this done by passing
down the information about whether the frame is constructed at the beginning of
the function or not. I only did it for arm64, it so happens (it was a
prototype).

Now... in this CL, I'm trying to lose any kind of notion of "was frame created
at beginning of function or not". So in that sense, I would probably want to
capture the frame state entering the OOL code. Can there be more than one code
path entering a OOL, btw? One where there is a frame setup, and one where there
isn't. I'll look into it.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.cc:97: masm()->InitializeRootRegister();
What's the root register? Seems like something that should happen together with
frame setup.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.cc:170: bool leaves_function =
On 2016/03/16 18:18:11, danno wrote:
> This should not be calculated here.  When the blocks are built, or when the
> frame elision analysis happens, must_decontruct_frame should be (re)set to
false
> for the block if the block ends in a deopt or return. That simplifies this
logic
> a lot, or at least puts the decision in a better place, and I think it makes
the
> decision much cleaner, you don't have to check every instruction, just the
last
> one. i.e. somewhere you need:
> 
> Instruction* instr = code()->InstructionAt(block->last_instruction_index())
> bool leaves_function = instr-> .... as before
> if (leaves_function) block->set_must_deconstruct_frame(false);

That makes sense, indeed. This is also a remnant of the older CL.

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

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.h:266: bool deconstruct_frame_when_leaving_;
On 2016/03/16 18:18:11, danno wrote:
> I don't think you want this variable here, I think it's much better
encapsulated
> in the frame_access_state. I would much prefer a boolean on the
> frame_access_state called currently_has_frame_. If starts out as false, and
you
> can query it. As you run through the blocks, you set it depending on
> must_construct/deconstruct frame.
> 
> There would also be a method called SetCurrentlyHasFrame, it would look
> something like this (maybe better in two methods, ConstructFrame and
> DeconstructFrame with appropriate asserts.
> 
> SetCurrentlyHasFrame(bool has_frame) {
>   has_frame_ = has_frame;
>   if (has_frame) {
>     SetFrameAccessToDefault();
>   } else {
>     SetFrameAccessToSP();
>   }
> }
> 
> GetSPToFPSlotCount would also be simpler, and for the most part remains
> unchanged.
> 
> GetSPToFPSlotCount() {
>  int used_slots = has_frame_ ? frame_->GetSPToFPSlotCount() :
> (kElidedFrameSlotCount - StandardFrameConstants::kFixedSlotCountAboveFp);
>  return used_slots + sp_delta()
> }
> 
> Then, at the end of the method, you just query
frame_access_state()->has_frame()
> to know if you need to deconstruct the frame.

I agree, this was from my 3 month old prototype.

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

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h#ne...
src/compiler/frame.h:217: return used_frame_slots() -
StandardFrameConstants::kFixedSlotCountAboveFp +
On 2016/03/16 18:18:11, danno wrote:
> I don't understand this change. The frame size frame_->GetSPToFPSlotCount()
> should be constant and set up knowing the the maximum slot count for the frame
> when it's present, regardless of whether there is a frame in this very moment
or
> not. 
> 
> So the implementation should make the decision something like this:
> 
> DCHECK(access_frame_with_fp());
> int frame_slot_count =  has_frame_ ? frame_->GetSPToFPSlotCount() :
> kElidedFrameSlotCount;
> return frame_slot_count + sp_delta();

Used_frame_slots is a crutch I needed to make sure I understood how things
worked. 

Having said that, let's talk  about Frame::GetSPToFPSlotCount. With frame
elision, the notion seems confusing to me, when exposed on Frame: what "FP" to
diff slots from SP? Turns out, there's only one consumer of this API, which is
here, on FrameAccessState. So I'd argue we remove the one on Frame, and have
just this one here, with the implementation you suggested.

Thoughts?

[Mircea Trofin, 2016-03-16 20:25:50]

On 2016/03/16 20:20:46, Mircea Trofin wrote:
> Thanks for the feedback, added some (and a question or 2) of my own. I'm
looking
> at the OOL stuff in the meantime.
> 
> Thanks!
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/arm/code-g...
> File src/compiler/arm/code-generator-arm.cc (right):
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/arm/code-g...
> src/compiler/arm/code-generator-arm.cc:247: if
> (!frame_access_state()->access_frame_with_fp()) {
> On 2016/03/16 18:18:11, danno wrote:
> > Here and below, and on other ARM-like platforms, I don't think is is
correct.
> > You can set up frame access with the SP even if you have a frame, this is
what
> > happens in tail calls and there is a bot that builds and runs all of the
tests
> > running with sp-based (not fp-based) frame access. You really need a
predicate
> > on the frame_access_state() called has_frame().
> 
> Ah, yes! I started from Intel and ended up cargo-culting the change from
> frame()->needs_frame() over. In  my old prototype, I had this done by passing
> down the information about whether the frame is constructed at the beginning
of
> the function or not. I only did it for arm64, it so happens (it was a
> prototype).
> 
> Now... in this CL, I'm trying to lose any kind of notion of "was frame created
> at beginning of function or not". So in that sense, I would probably want to
> capture the frame state entering the OOL code. Can there be more than one code
> path entering a OOL, btw? One where there is a frame setup, and one where
there
> isn't. I'll look into it.
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
> File src/compiler/code-generator.cc (right):
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
> src/compiler/code-generator.cc:97: masm()->InitializeRootRegister();
> What's the root register? Seems like something that should happen together
with
> frame setup.
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
> src/compiler/code-generator.cc:170: bool leaves_function =
> On 2016/03/16 18:18:11, danno wrote:
> > This should not be calculated here.  When the blocks are built, or when the
> > frame elision analysis happens, must_decontruct_frame should be (re)set to
> false
> > for the block if the block ends in a deopt or return. That simplifies this
> logic
> > a lot, or at least puts the decision in a better place, and I think it makes
> the
> > decision much cleaner, you don't have to check every instruction, just the
> last
> > one. i.e. somewhere you need:
> > 
> > Instruction* instr = code()->InstructionAt(block->last_instruction_index())
> > bool leaves_function = instr-> .... as before
> > if (leaves_function) block->set_must_deconstruct_frame(false);
> 
> That makes sense, indeed. This is also a remnant of the older CL.
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
> File src/compiler/code-generator.h (right):
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
> src/compiler/code-generator.h:266: bool deconstruct_frame_when_leaving_;
> On 2016/03/16 18:18:11, danno wrote:
> > I don't think you want this variable here, I think it's much better
> encapsulated
> > in the frame_access_state. I would much prefer a boolean on the
> > frame_access_state called currently_has_frame_. If starts out as false, and
> you
> > can query it. As you run through the blocks, you set it depending on
> > must_construct/deconstruct frame.
> > 
> > There would also be a method called SetCurrentlyHasFrame, it would look
> > something like this (maybe better in two methods, ConstructFrame and
> > DeconstructFrame with appropriate asserts.
> > 
> > SetCurrentlyHasFrame(bool has_frame) {
> >   has_frame_ = has_frame;
> >   if (has_frame) {
> >     SetFrameAccessToDefault();
> >   } else {
> >     SetFrameAccessToSP();
> >   }
> > }
> > 
> > GetSPToFPSlotCount would also be simpler, and for the most part remains
> > unchanged.
> > 
> > GetSPToFPSlotCount() {
> >  int used_slots = has_frame_ ? frame_->GetSPToFPSlotCount() :
> > (kElidedFrameSlotCount - StandardFrameConstants::kFixedSlotCountAboveFp);
> >  return used_slots + sp_delta()
> > }
> > 
> > Then, at the end of the method, you just query
> frame_access_state()->has_frame()
> > to know if you need to deconstruct the frame.
> 
> I agree, this was from my 3 month old prototype.
> 
> https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h
> File src/compiler/frame.h (right):
> 
>
https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h#ne...
> src/compiler/frame.h:217: return used_frame_slots() -
> StandardFrameConstants::kFixedSlotCountAboveFp +
> On 2016/03/16 18:18:11, danno wrote:
> > I don't understand this change. The frame size frame_->GetSPToFPSlotCount()
> > should be constant and set up knowing the the maximum slot count for the
frame
> > when it's present, regardless of whether there is a frame in this very
moment
> or
> > not. 
> > 
> > So the implementation should make the decision something like this:
> > 
> > DCHECK(access_frame_with_fp());
> > int frame_slot_count =  has_frame_ ? frame_->GetSPToFPSlotCount() :
> > kElidedFrameSlotCount;
> > return frame_slot_count + sp_delta();
> 
> Used_frame_slots is a crutch I needed to make sure I understood how things
> worked. 
> 
> Having said that, let's talk  about Frame::GetSPToFPSlotCount. With frame
> elision, the notion seems confusing to me, when exposed on Frame: what "FP" to
> diff slots from SP? Turns out, there's only one consumer of this API, which is
> here, on FrameAccessState. So I'd argue we remove the one on Frame, and have
> just this one here, with the implementation you suggested.
> 
> Thoughts?

FWIW - I plan to have a CL following this one where the Frame lifetime is 
more clearly delineated. It appears we have a build-up phase, where all the 
parameters are determined, followed by codegen, where the frame should stay
immutable and we only use frame_access_state (the comments on Frame
say this, too). Perhaps we want a MutableFrame and a Frame (immutable)? Anyway,
subsequent CL.

[danno, 2016-03-16 21:20:57]

some more feedback and answers to your questions

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.cc:97: masm()->InitializeRootRegister();
On 2016/03/16 20:20:46, Mircea Trofin wrote:
> What's the root register? Seems like something that should happen together
with
> frame setup.

Nah, this is unrelated to the frame and should stay. This is the register that
common constants are loaded from, and just needs to be set up once in the
prologue.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.cc:797: bool has_frame =
descriptor->IsJSFunctionCall() || spill_slots > 0;
by the way, this will probably end up being 

bool has_frame = frame_access_state->has_frame(), and you will need to
initialize that appropriately based on the prologue based on the logic that is
already there which is dangerously similar to the code above, but in fact the
logic here is probably wrong, it should be _exactly_ the condition which causes
the prologue frame to be generated... thus switching to 
frame_access_state->has_frame() is much "safer", too.

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.cc:801: has_frame ?
(frame_access_state()->used_frame_slots() - pc_slots) : 0;
If you use frame_access_state->has_frame(), you can keep the logic as it was
before.

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

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/frame.h#ne...
src/compiler/frame.h:217: return used_frame_slots() -
StandardFrameConstants::kFixedSlotCountAboveFp +
On 2016/03/16 20:20:46, Mircea Trofin wrote:
> On 2016/03/16 18:18:11, danno wrote:
> > I don't understand this change. The frame size frame_->GetSPToFPSlotCount()
> > should be constant and set up knowing the the maximum slot count for the
frame
> > when it's present, regardless of whether there is a frame in this very
moment
> or
> > not. 
> > 
> > So the implementation should make the decision something like this:
> > 
> > DCHECK(access_frame_with_fp());
> > int frame_slot_count =  has_frame_ ? frame_->GetSPToFPSlotCount() :
> > kElidedFrameSlotCount;
> > return frame_slot_count + sp_delta();
> 
> Used_frame_slots is a crutch I needed to make sure I understood how things
> worked. 
> 
> Having said that, let's talk  about Frame::GetSPToFPSlotCount. With frame
> elision, the notion seems confusing to me, when exposed on Frame: what "FP" to
> diff slots from SP? Turns out, there's only one consumer of this API, which is
> here, on FrameAccessState. So I'd argue we remove the one on Frame, and have
> just this one here, with the implementation you suggested.
> 
> Thoughts?

Yeah, I can see that now. That sounds OK.

[Mircea Trofin, 2016-03-16 21:51:00]

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/20001/src/compiler/code-gener...
src/compiler/code-generator.cc:97: masm()->InitializeRootRegister();
On 2016/03/16 21:20:56, danno wrote:
> On 2016/03/16 20:20:46, Mircea Trofin wrote:
> > What's the root register? Seems like something that should happen together
> with
> > frame setup.
> 
> Nah, this is unrelated to the frame and should stay. This is the register that
> common constants are loaded from, and just needs to be set up once in the
> prologue.

it seems that Simulator::CheckPCSComplianceAndRun cares about this ordering,
though.
Probably best to move it right after AssemblePrologue, at least for now.

[Mircea Trofin, 2016-03-17 00:28:30]

Patchset #2 (id:40001) has been deleted

[Mircea Trofin, 2016-03-20 16:01:06]

Description was changed from

==========
Frame elision for code stubs

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

to

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

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

[Mircea Trofin, 2016-03-20 16:15:23]

This CL includes changes reviewed separately:

1818503002 [turbofan] PropagateMarks until we have no more change.
1810333003 [turbofan] Move frame elision logic to the end
1811213003 [turbofan] Validation for deferred->hot transition edges

And depends on 1811333002: Ensure Schedules generated by the 
RawMachineAssembler are in edge-split form.

I'm sending it for feedback on the elision mechanism.

Once this is in, I intend to:
- add a test for the mechanism itself
- add unit testing for FrameElider
- move away any Frame mutation to just before code generation. Right
now, there's some mutation possible when assembling prologue, 
just that it won't happen when we elide frames because it is not 
applicable.

[danno, 2016-03-22 18:10:09]

First round of feedback.

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/code-gene...
src/compiler/code-generator.cc:152: Instruction* instr =
code()->InstructionAt(i);
This is way more complicated than it needs to be AFAICT. I think you can just
get rid of this code ...

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/code-gene...
src/compiler/code-generator.cc:159: AssembleDeconstructFrameIfNeeded();
And directly here:

if (block->must_deconstruct_frame()) {
  AssembleDeconstructFrame();
}

Then you can get rid of the AssembleDeconstructFrameIfNeeded routine and the
boolean state associated with the decision in the frame state.

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/code-gene...
src/compiler/code-generator.cc:800: // DCHECK(has_frame ==
frame_access_state()->frame_enabled());
I think you can remove this commented-out code.

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

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/frame.h#n...
src/compiler/frame.h:217: void set_deconstruct_frame_if_needed(bool deconstruct)
{
I don't think this is needed, either. See code-generator.cc

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/frame.h#n...
src/compiler/frame.h:222: void MarkReturnBlockDeconstructs() {
I don't think belongs on the frame_access_state, and I don't think it's what you
really want anyway.

I think what you want belongs on the code-generator, and it probably should look
different. Basically, you want to have two possible return labels, one that
deconstructs the frame, one that does not. Generate both, and jump to either the
one with or without frame destruction depending on the context you're coming
from.

[Mircea Trofin, 2016-03-22 19:04:09]

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/270001/src/compiler/code-gene...
src/compiler/code-generator.cc:159: AssembleDeconstructFrameIfNeeded();
On 2016/03/22 18:10:09, danno wrote:
> And directly here:
> 
> if (block->must_deconstruct_frame()) {
>   AssembleDeconstructFrame();
> }
> 
> Then you can get rid of the AssembleDeconstructFrameIfNeeded routine and the
> boolean state associated with the decision in the frame state.

The problem is the instruction being assembled. I may be a conditional branch.
We need to 1) assemble the condition (which may involve accessing the frame), 2)
disassemble the frame, 3) jump. That's why I think we can't avoid
AssembleDeconstructFrameIfNeeded.

[Mircea Trofin, 2016-03-23 05:51:54]

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

[danno, 2016-03-23 07:41:01]

Unless I'm missing something, this isn't a problem that you can encounter in
practice because the graph is in edge-split form. Because any multiple-target
branch will be followed by blocks that only have a single predecessor (that's
the guarantee of edge splitting), if you propagate the frame deconstruction
forward as far as possible without entering a non-deferred block, you will
always have a place to put the deconstruction in a block that doesn't have
multiple successors. This is an invariant you can/should probably check. 

In essence, this is just like phi resolution. Just like edge-split form ensures
that you can always put unique phi resolves at the end of blocks before a path
merge without having to specially handle branches, it also should allow you to
do frame deconstruction without the special handling.

Make sense? Or am I out of my mind? :-)

[Mircea Trofin, 2016-03-23 07:47:22]

On 2016/03/23 07:41:01, danno wrote:
> Unless I'm missing something, this isn't a problem that you can encounter in
> practice because the graph is in edge-split form. Because any multiple-target
> branch will be followed by blocks that only have a single predecessor (that's
> the guarantee of edge splitting), if you propagate the frame deconstruction
> forward as far as possible without entering a non-deferred block, you will
> always have a place to put the deconstruction in a block that doesn't have
> multiple successors. This is an invariant you can/should probably check. 
> 
> In essence, this is just like phi resolution. Just like edge-split form
ensures
> that you can always put unique phi resolves at the end of blocks before a path
> merge without having to specially handle branches, it also should allow you to
> do frame deconstruction without the special handling.
> 
> Make sense? Or am I out of my mind? :-)

Hmm. That makes sense. I also remember having to go to this design for a reason,

I wonder if it's because I hit the issue you just committed, where the single 
successor/predecessor blocks exiting out of the deferred path weren't deferred.

I'll give this a try (tomorrow)

[Mircea Trofin, 2016-03-23 16:18:51]

On 2016/03/23 07:47:22, Mircea Trofin wrote:
> On 2016/03/23 07:41:01, danno wrote:
> > Unless I'm missing something, this isn't a problem that you can encounter in
> > practice because the graph is in edge-split form. Because any
multiple-target
> > branch will be followed by blocks that only have a single predecessor
(that's
> > the guarantee of edge splitting), if you propagate the frame deconstruction
> > forward as far as possible without entering a non-deferred block, you will
> > always have a place to put the deconstruction in a block that doesn't have
> > multiple successors. This is an invariant you can/should probably check. 
> > 
> > In essence, this is just like phi resolution. Just like edge-split form
> ensures
> > that you can always put unique phi resolves at the end of blocks before a
path
> > merge without having to specially handle branches, it also should allow you
to
> > do frame deconstruction without the special handling.
> > 
> > Make sense? Or am I out of my mind? :-)
> 
> Hmm. That makes sense. I also remember having to go to this design for a
reason,
> 
> I wonder if it's because I hit the issue you just committed, where the single 
> successor/predecessor blocks exiting out of the deferred path weren't
deferred.
> 
> I'll give this a try (tomorrow)

I remember now what the issue is. The last instruction may have gap moves that 
reference stack operands. Still, I can simplify the design quite a bit by just 
adding the disassembling logic in the common code generator, and maybe 
passing a bool to AssembleInstruction, rather than this extra state machinery.

[Mircea Trofin, 2016-03-24 04:22:49]

Patchset #17 (id:350001) has been deleted

[Mircea Trofin, 2016-03-24 04:23:02]

Patchset #18 (id:390001) has been deleted

[Mircea Trofin, 2016-03-24 04:23:15]

Patchset #17 (id:370001) has been deleted

[Mircea Trofin, 2016-03-24 04:23:33]

Patchset #16 (id:330001) has been deleted

[danno, 2016-03-24 12:53:06]

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/arm64/cod...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1478: void
CodeGenerator::SetupStackPointer() {
If wonder if this should be called AssemblePrologue if what is currently
AssemblePrologue become AssembleConstructFrame

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1489: frame()->AlignFrame(16);
Shouldn't you remove this now?

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.cc:145: AssemblePrologue();
You should rename this to AssembleConstructFrame

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.cc:151: for (int i = block->code_start(); i <
block->last_instruction_index();
I think it would be slightly cleaner if you keep the original single loop and
check against the index in the loop, e.g.:

Instruction* last_instr =
code()->InstructionAt(block->last_instruction_index());
for (int i = block->code_start(); i < block->code_end(); ++i) {
  Instruction* instr = code()->InstructionAt(i);
  AssembleGaps(instr);
  if (instr == last_instr && block->must_deconstruct_frame() && !instr->IsRet())
{
   AssembleDeconstructFrame(instr)
  }
  AssembleInstruction();
}

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.cc:169: AssembleDeconstructFrame();
In what case does this actually happen? It seems to not be symmetric and
suggests something else is not quite right. Every block that can deconstruction
must end either in jump or a ret, everything else (switch, tail call, branch)
should never end up here, and therefore the code I suggest above should be
sufficient, right?

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.cc:789: // DCHECK(has_frame ==
frame_access_state()->frame_enabled());
Please remove the commented-out code.

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
File src/compiler/code-generator.h (left):

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.h:60: 
nit: Can you please add this line back? Seems like an unrelated and unnecessary
change.

[Mircea Trofin, 2016-03-24 17:09:48]

Patchset #17 (id:430001) has been deleted

[Mircea Trofin, 2016-03-24 17:10:08]

Patchset #17 (id:450001) has been deleted

[Mircea Trofin, 2016-03-24 17:46:12]

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/arm64/cod...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1478: void
CodeGenerator::SetupStackPointer() {
On 2016/03/24 12:53:05, danno wrote:
> If wonder if this should be called AssemblePrologue if what is currently
> AssemblePrologue become AssembleConstructFrame

Possibly. I want to restructure AssemblePrologue because it does a few things,
including mutating the Frame (in
some cases). I'd prefer doing this rename with that restructuring.

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1489: frame()->AlignFrame(16);
On 2016/03/24 12:53:05, danno wrote:
> Shouldn't you remove this now?

Done.

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.cc:145: AssemblePrologue();
On 2016/03/24 12:53:05, danno wrote:
> You should rename this to AssembleConstructFrame

Addressed in first comment.

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.cc:789: // DCHECK(has_frame ==
frame_access_state()->frame_enabled());
On 2016/03/24 12:53:05, danno wrote:
> Please remove the commented-out code.

Done.

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
File src/compiler/code-generator.h (left):

https://codereview.chromium.org/1775323002/diff/410001/src/compiler/code-gene...
src/compiler/code-generator.h:60: 
On 2016/03/24 12:53:05, danno wrote:
> nit: Can you please add this line back? Seems like an unrelated and
unnecessary
> change.

Done.

[Benedikt Meurer, 2016-03-24 19:04:05]

First round of comments. Looking good on first sight.

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/arm/code-...
File src/compiler/arm/code-generator-arm.cc (left):

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/arm/code-...
src/compiler/arm/code-generator-arm.cc:1352: // Canonicalize JSFunction return
sites for now.
Nit: Please put back this comment (on other archs too).

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
File src/compiler/code-generator.cc (left):

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
src/compiler/code-generator.cc:85: masm()->InitializeRootRegister();
I don't understand this code removal: The root register is independent of the
frame (and vice versa). I.e. code in frame-less function/section can still
access the roots.

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
src/compiler/code-generator.cc:147: masm()->InitializeRootRegister();
See comment above.

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

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
src/compiler/code-generator.h:110: void SetupStackPointer();
Nit: Please follow the naming convection for assembling methods (AssembleFoo),
i.e. AssembleSetStackPointer or something like that.

[Mircea Trofin, 2016-03-24 19:58:07]

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/arm/code-...
File src/compiler/arm/code-generator-arm.cc (left):

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/arm/code-...
src/compiler/arm/code-generator-arm.cc:1352: // Canonicalize JSFunction return
sites for now.
On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> Nit: Please put back this comment (on other archs too).

Done.

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
File src/compiler/code-generator.cc (left):

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
src/compiler/code-generator.cc:85: masm()->InitializeRootRegister();
On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> I don't understand this code removal: The root register is independent of the
> frame (and vice versa). I.e. code in frame-less function/section can still
> access the roots.

I thought so, too, but the arm simulator gets upset with this here, in
Simulator::CheckPCSComplianceAndRun.

I left a TODO to check why this is the case. Unless it's a big surprise/concern,
I'd prefer doing that separately.

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
src/compiler/code-generator.cc:147: masm()->InitializeRootRegister();
On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> See comment above.

(answered above)

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

https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
src/compiler/code-generator.h:110: void SetupStackPointer();
On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> Nit: Please follow the naming convection for assembling methods (AssembleFoo),
> i.e. AssembleSetStackPointer or something like that.

Done.

[Mircea Trofin, 2016-03-25 17:19:36]

Description was changed from

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

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

to

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG= v8:4533
==========

[Mircea Trofin, 2016-03-29 14:26:55]

On 2016/03/24 19:58:07, Mircea Trofin wrote:
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/arm/code-...
> File src/compiler/arm/code-generator-arm.cc (left):
> 
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/arm/code-...
> src/compiler/arm/code-generator-arm.cc:1352: // Canonicalize JSFunction return
> sites for now.
> On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> > Nit: Please put back this comment (on other archs too).
> 
> Done.
> 
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
> File src/compiler/code-generator.cc (left):
> 
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
> src/compiler/code-generator.cc:85: masm()->InitializeRootRegister();
> On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> > I don't understand this code removal: The root register is independent of
the
> > frame (and vice versa). I.e. code in frame-less function/section can still
> > access the roots.
> 
> I thought so, too, but the arm simulator gets upset with this here, in
> Simulator::CheckPCSComplianceAndRun.
> 
> I left a TODO to check why this is the case. Unless it's a big
surprise/concern,
> I'd prefer doing that separately.
> 
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
> File src/compiler/code-generator.cc (right):
> 
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
> src/compiler/code-generator.cc:147: masm()->InitializeRootRegister();
> On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> > See comment above.
> 
> (answered above)
> 
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
> File src/compiler/code-generator.h (right):
> 
>
https://codereview.chromium.org/1775323002/diff/470001/src/compiler/code-gene...
> src/compiler/code-generator.h:110: void SetupStackPointer();
> On 2016/03/24 19:04:05, Benedikt Meurer wrote:
> > Nit: Please follow the naming convection for assembling methods
(AssembleFoo),
> > i.e. AssembleSetStackPointer or something like that.
> 
> Done.

Gentle reminder.

[danno, 2016-03-29 15:04:28]

Getting very close. If think if you address my comments, you'll be good to go
from my perspective.

https://codereview.chromium.org/1775323002/diff/510001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/510001/src/compiler/code-gene...
src/compiler/code-generator.cc:149: masm()->InitializeRootRegister();
I'd really like to know why this doesn't work when it's in AssemblePrologue. It
makes me a bit nervous that this special handling needs to be in here and we
don't understand exactly why, and ideally I would like to have it fixed before
landing this.

https://codereview.chromium.org/1775323002/diff/510001/src/compiler/code-gene...
src/compiler/code-generator.cc:156: if (block->must_deconstruct_frame() &&
Can you add a predicate to block called MustDeconstructFrameAfterInstruction()
that takes index i and encapsulates this test?

[Mircea Trofin, 2016-03-29 16:28:56]

Description was changed from

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG= v8:4533
==========

to

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG= v8:4533
LOG=N
==========

[Mircea Trofin, 2016-03-29 23:59:30]

Patchset #21 (id:540001) has been deleted

[Benedikt Meurer, 2016-03-30 04:00:25]

Getting there.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:149: AssemblePrologue();
Maybe we should rename AssemblePrologue to AssembleConstructFrame for
consistency? Then it would be obvious that these two are dual.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:153: Instruction* instr =
code()->InstructionAt(i);
Please put all of this into AssembleInstruction below and let the for-loop body
be just the call to AssembleInstruction.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:155: if (block->must_deconstruct_frame() &&
This condition looks unnecessarily complex. Isn't what you actually want just

if (instr->IsJump() && block->must_deconstruct_frame()) {
  DCHECK_EQ(block->last_instruction_index(), i);
  DCHECK(!instr->IsRet());
  ...
}

or am I missing something? Also even for instr->IsRet() you probably need to
deconstruct the frame somehow (I guess that's part of AssembleReturn tho).

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

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.h:110: void AssembleSetupStackPointer();
Nit: Add comment to explain what this is supposed to do.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.h:119: void AssembleDeconstructFrame();
Nit: Add comment to explain what this is supposed to do.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.cc (right):

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:656: Label if_overflow(this,
Label::kDeferred), if_notoverflow(this);
Meh C++ should warn about this, really. But this should be in a separate CL.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:664: Goto(&if_valueisheapnumber);
This change looks completely unrelated and wrong to me. This way you'd even box
values in the Smi range into a HeapNumber. Why did you change this?

[Mircea Trofin, 2016-03-30 04:13:27]

https://codereview.chromium.org/1775323002/diff/510001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/510001/src/compiler/code-gene...
src/compiler/code-generator.cc:156: if (block->must_deconstruct_frame() &&
On 2016/03/29 15:04:28, danno wrote:
> Can you add a predicate to block called MustDeconstructFrameAfterInstruction()
> that takes index i and encapsulates this test?

I think the main problem here is this !instr->IsRet(). Without it, the rest
would look pretty clean and straight forward. I'd prefer refactoring (subsequent
to this CL) AssembleReturn, so it won't disassemble the frame 
by itself.

Or were you talking about just the first 2 conditions?

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:149: AssemblePrologue();
On 2016/03/30 04:00:25, Benedikt Meurer wrote:
> Maybe we should rename AssemblePrologue to AssembleConstructFrame for
> consistency? Then it would be obvious that these two are dual.

I want to do that, but together with separating frame mutation out of it, so in
a separate CL.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:153: Instruction* instr =
code()->InstructionAt(i);
On 2016/03/30 04:00:25, Benedikt Meurer wrote:
> Please put all of this into AssembleInstruction below and let the for-loop
body
> be just the call to AssembleInstruction.

We need the operands involved in the gap to be emitted before the frame is
deconstructed.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:155: if (block->must_deconstruct_frame() &&
On 2016/03/30 04:00:25, Benedikt Meurer wrote:
> This condition looks unnecessarily complex. Isn't what you actually want just
> 
> if (instr->IsJump() && block->must_deconstruct_frame()) {
>   DCHECK_EQ(block->last_instruction_index(), i);
>   DCHECK(!instr->IsRet());
>   ...
> }
> 
> or am I missing something? Also even for instr->IsRet() you probably need to
> deconstruct the frame somehow (I guess that's part of AssembleReturn tho).

That's true for ret, and I plan to address that separately. I agree with the
condition change.

[Benedikt Meurer, 2016-03-30 04:34:08]

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:149: AssemblePrologue();
On 2016/03/30 04:13:27, Mircea Trofin wrote:
> On 2016/03/30 04:00:25, Benedikt Meurer wrote:
> > Maybe we should rename AssemblePrologue to AssembleConstructFrame for
> > consistency? Then it would be obvious that these two are dual.
> 
> I want to do that, but together with separating frame mutation out of it, so
in
> a separate CL.

Acknowledged.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:153: Instruction* instr =
code()->InstructionAt(i);
Sure, but that can all be done inside AssembleInstruction, which previously
contained the AssembleGaps call.

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:155: if (block->must_deconstruct_frame() &&
On 2016/03/30 04:13:27, Mircea Trofin wrote:
> On 2016/03/30 04:00:25, Benedikt Meurer wrote:
> > This condition looks unnecessarily complex. Isn't what you actually want
just
> > 
> > if (instr->IsJump() && block->must_deconstruct_frame()) {
> >   DCHECK_EQ(block->last_instruction_index(), i);
> >   DCHECK(!instr->IsRet());
> >   ...
> > }
> > 
> > or am I missing something? Also even for instr->IsRet() you probably need to
> > deconstruct the frame somehow (I guess that's part of AssembleReturn tho).
> 
> That's true for ret, and I plan to address that separately. I agree with the
> condition change.

Acknowledged.

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

As discussed offline, for now just move the initialization of the root register
to avoid the issue with the callee saved registers in case of C linkage and
using the roots.

[Mircea Trofin, 2016-03-30 05:58:20]

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1775323002/diff/580001/src/compiler/code-gene...
src/compiler/code-generator.cc:153: Instruction* instr =
code()->InstructionAt(i);
On 2016/03/30 04:34:08, Benedikt Meurer wrote:
> Sure, but that can all be done inside AssembleInstruction, which previously
> contained the AssembleGaps call.

Done.

[danno, 2016-03-30 10:49:19]

nice. lgtm.

[Benedikt Meurer, 2016-03-30 10:50:24]

LGTM.

[Mircea Trofin, 2016-03-30 14:06:01]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-03-30 14:06:06]

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

[commit-bot: I haz the power, 2016-03-30 14:08:07]

Description was changed from

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG= v8:4533
LOG=N
==========

to

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG= v8:4533
LOG=N
==========

[commit-bot: I haz the power, 2016-03-30 14:08:09]

Committed patchset #23 (id:600001)

[commit-bot: I haz the power, 2016-03-30 14:08:36]

Description was changed from

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG= v8:4533
LOG=N
==========

to

==========
[turbofan] Frame elision for code stubs.

Removed Frame::needs_frame and the function-wide logic using it in
favor of FrameAccessState::has_frame, which can be set on a more
granular level, and driving it block by block.

BUG= v8:4533
LOG=N

Committed: https://crrev.com/53d51c52f3754fe0e7decacba4277e58cf9fb5a4
Cr-Commit-Position: refs/heads/master@{#35139}
==========

[commit-bot: I haz the power, 2016-03-30 14:08:37]

Patchset 23 (id:??) landed as
https://crrev.com/53d51c52f3754fe0e7decacba4277e58cf9fb5a4
Cr-Commit-Position: refs/heads/master@{#35139}

[Benedikt Meurer, 2016-03-31 06:48:36]

Frame elision works great, TurboFan code stubs like BitwiseOrStub, ToLengthStub
and friends are close to perfect for the common case now. Thanks!

[Mircea Trofin, 2016-03-31 07:03:14]

On 2016/03/31 06:48:36, Benedikt Meurer (OOO) wrote:
> Frame elision works great, TurboFan code stubs like BitwiseOrStub,
ToLengthStub
> and friends are close to perfect for the common case now. Thanks!

Glad to hear!

What is left to make them perfect?