Do stack checks while pushing locals

Do stack checks while pushing locals

R=yangguo@chromium.org

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=20176

[dcarney, 2014-03-21 09:43:27]

ptal

https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:191:
PrepareForBailoutForId(BailoutId::FunctionEntry(), NO_REGISTERS);
no idea what this does, whether it's the right bailout id, or whether i can call
it twice

[Yang, 2014-03-21 09:52:22]

LGTM.

https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:191:
PrepareForBailoutForId(BailoutId::FunctionEntry(), NO_REGISTERS);
On 2014/03/21 09:43:28, dcarney wrote:
> no idea what this does, whether it's the right bailout id, or whether i can
call
> it twice

I don't think we need this. I don't expect optimized code to somehow deoptimize
and end up here. (At least I think that's what this is for).

https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:204: __ cmp(ecx, Immediate(Smi::FromInt(0)));
No need for this cmp. dec also sets the flags. Just do a j(not_zero,
&loop_header) later. (not_zero is actually the same flag as not_equal).

[Yang, 2014-03-21 12:26:54]

On 2014/03/21 09:52:22, Yang wrote:
> LGTM.
> 
> https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.cc
> File src/ia32/full-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> src/ia32/full-codegen-ia32.cc:191:
> PrepareForBailoutForId(BailoutId::FunctionEntry(), NO_REGISTERS);
> On 2014/03/21 09:43:28, dcarney wrote:
> > no idea what this does, whether it's the right bailout id, or whether i can
> call
> > it twice
> 
> I don't think we need this. I don't expect optimized code to somehow
deoptimize
> and end up here. (At least I think that's what this is for).
> 
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> src/ia32/full-codegen-ia32.cc:204: __ cmp(ecx, Immediate(Smi::FromInt(0)));
> No need for this cmp. dec also sets the flags. Just do a j(not_zero,
> &loop_header) later. (not_zero is actually the same flag as not_equal).

LGTM, but...

I kinda feel bad about this, but at the time we do the first stack check, we can
already know whether there is going to be a stack overflow, simply by comparing
(esp - locals_count) with the stack limit. We don't have to do it in every loop.

Having the loop there is a good idea nevertheless!

[Yang, 2014-03-21 12:27:34]

On 2014/03/21 12:26:54, Yang wrote:
> On 2014/03/21 09:52:22, Yang wrote:
> > LGTM.
> > 
> >
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.cc
> > File src/ia32/full-codegen-ia32.cc (right):
> > 
> >
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> > src/ia32/full-codegen-ia32.cc:191:
> > PrepareForBailoutForId(BailoutId::FunctionEntry(), NO_REGISTERS);
> > On 2014/03/21 09:43:28, dcarney wrote:
> > > no idea what this does, whether it's the right bailout id, or whether i
can
> > call
> > > it twice
> > 
> > I don't think we need this. I don't expect optimized code to somehow
> deoptimize
> > and end up here. (At least I think that's what this is for).
> > 
> >
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> > src/ia32/full-codegen-ia32.cc:204: __ cmp(ecx, Immediate(Smi::FromInt(0)));
> > No need for this cmp. dec also sets the flags. Just do a j(not_zero,
> > &loop_header) later. (not_zero is actually the same flag as not_equal).
> 
> LGTM, but...
> 
> I kinda feel bad about this, but at the time we do the first stack check, we
can
> already know whether there is going to be a stack overflow, simply by
comparing
> (esp - locals_count) with the stack limit. We don't have to do it in every
loop.
> 
> Having the loop there is a good idea nevertheless!

(I was feeling bad about this for not noticing earlier and having you port to
all platforms already).

[dcarney, 2014-03-21 15:17:57]

On 2014/03/21 12:27:34, Yang wrote:
> On 2014/03/21 12:26:54, Yang wrote:
> > On 2014/03/21 09:52:22, Yang wrote:
> > > LGTM.
> > > 
> > >
> https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.cc
> > > File src/ia32/full-codegen-ia32.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> > > src/ia32/full-codegen-ia32.cc:191:
> > > PrepareForBailoutForId(BailoutId::FunctionEntry(), NO_REGISTERS);
> > > On 2014/03/21 09:43:28, dcarney wrote:
> > > > no idea what this does, whether it's the right bailout id, or whether i
> can
> > > call
> > > > it twice
> > > 
> > > I don't think we need this. I don't expect optimized code to somehow
> > deoptimize
> > > and end up here. (At least I think that's what this is for).
> > > 
> > >
> >
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> > > src/ia32/full-codegen-ia32.cc:204: __ cmp(ecx,
Immediate(Smi::FromInt(0)));
> > > No need for this cmp. dec also sets the flags. Just do a j(not_zero,
> > > &loop_header) later. (not_zero is actually the same flag as not_equal).
> > 
> > LGTM, but...
> > 
> > I kinda feel bad about this, but at the time we do the first stack check, we
> can
> > already know whether there is going to be a stack overflow, simply by
> comparing
> > (esp - locals_count) with the stack limit. We don't have to do it in every
> loop.
> > 
> > Having the loop there is a good idea nevertheless!
> 
> (I was feeling bad about this for not noticing earlier and having you port to
> all platforms already).

new version with check before loop

[Yang, 2014-03-24 07:55:48]

On 2014/03/21 15:17:57, dcarney wrote:
> On 2014/03/21 12:27:34, Yang wrote:
> > On 2014/03/21 12:26:54, Yang wrote:
> > > On 2014/03/21 09:52:22, Yang wrote:
> > > > LGTM.
> > > > 
> > > >
> >
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.cc
> > > > File src/ia32/full-codegen-ia32.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> > > > src/ia32/full-codegen-ia32.cc:191:
> > > > PrepareForBailoutForId(BailoutId::FunctionEntry(), NO_REGISTERS);
> > > > On 2014/03/21 09:43:28, dcarney wrote:
> > > > > no idea what this does, whether it's the right bailout id, or whether
i
> > can
> > > > call
> > > > > it twice
> > > > 
> > > > I don't think we need this. I don't expect optimized code to somehow
> > > deoptimize
> > > > and end up here. (At least I think that's what this is for).
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/207543003/diff/1/src/ia32/full-codegen-ia32.c...
> > > > src/ia32/full-codegen-ia32.cc:204: __ cmp(ecx,
> Immediate(Smi::FromInt(0)));
> > > > No need for this cmp. dec also sets the flags. Just do a j(not_zero,
> > > > &loop_header) later. (not_zero is actually the same flag as not_equal).
> > > 
> > > LGTM, but...
> > > 
> > > I kinda feel bad about this, but at the time we do the first stack check,
we
> > can
> > > already know whether there is going to be a stack overflow, simply by
> > comparing
> > > (esp - locals_count) with the stack limit. We don't have to do it in every
> > loop.
> > > 
> > > Having the loop there is a good idea nevertheless!
> > 
> > (I was feeling bad about this for not noticing earlier and having you port
to
> > all platforms already).
> 
> new version with check before loop

LGTM.

[dcarney, 2014-03-24 08:17:40]

Committed patchset #8 manually as r20176 (presubmit successful).