MIPS: Reland r21442 "Inobject slack tracking is done on a per-closure basis instead of per-shared i…

MIPS: Reland r21442 "Inobject slack tracking is done on a per-closure basis instead of per-shared info basis."

Port r21457 (8db39a8)

Original commit message:
This fixes inobject slack tracking for prototype inheritance pattern that uses closures.

BUG=

[Paul Lind, 2014-05-23 15:01:44]

One comment, PTAL

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:440: Label no_inobject_slack_tracking;
The deleted LoadRoot for UndefinedValue needs to be added above this if clause -
so  its available on all possible paths to  __ InitializeFieldsWithFiller(t5,
a0, t7)

[Igor Sheludko, 2014-05-23 15:04:18]

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:390: __ DecodeField<Map::ConstructionCount>(a3, t0);
What do you think about storing decoded counter value in t8 in order to avoid...

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:440: Label no_inobject_slack_tracking;
On 2014/05/23 15:01:45, Paul Lind wrote:
> The deleted LoadRoot for UndefinedValue needs to be added above this if clause
-
> so  its available on all possible paths to  __ InitializeFieldsWithFiller(t5,
> a0, t7)

There is one load at the top of the function. I think it is better to move it
here, before the if().

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:444: __ DecodeField<Map::ConstructionCount>(t0);
... reloading here, like it was done for ia32?

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:453: __ Addu(t6, t4, Operand(at));   // End of object.
I think we can now move above two lines inside the if() block below.

[Paul Lind, 2014-05-23 15:33:39]

Thanks for detailed review Igor!

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:390: __ DecodeField<Map::ConstructionCount>(a3, t0);
On 2014/05/23 15:04:18, Igor Sheludko wrote:
> What do you think about storing decoded counter value in t8 in order to
avoid...

t8 is not good for this, it's reserved for the macro-assembler. (we use it
occasionally as short-lived temp, as in line 557). t2 seems available in this
function, we could use that.

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:440: Label no_inobject_slack_tracking;
On 2014/05/23 15:04:18, Igor Sheludko wrote:
> On 2014/05/23 15:01:45, Paul Lind wrote:
> > The deleted LoadRoot for UndefinedValue needs to be added above this if
clause
> -
> > so  its available on all possible paths to  __
InitializeFieldsWithFiller(t5,
> > a0, t7)
> 
> There is one load at the top of the function. I think it is better to move it
> here, before the if().

I'd missed that one. I agree, better to move it here.

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:444: __ DecodeField<Map::ConstructionCount>(t0);
On 2014/05/23 15:04:18, Igor Sheludko wrote:
> ... reloading here, like it was done for ia32?

Sure, we can do that with t2. Balázs, please add comment about value in t2:
slack tracking counter (non-API function case)

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:453: __ Addu(t6, t4, Operand(at));   // End of object.
On 2014/05/23 15:04:18, Igor Sheludko wrote:
> I think we can now move above two lines inside the if() block below.

Agree, good catch.

[kilvadyb, 2014-05-23 17:51:50]

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:390: __ DecodeField<Map::ConstructionCount>(a3, t0);
On 2014/05/23 15:33:39, Paul Lind wrote:
> On 2014/05/23 15:04:18, Igor Sheludko wrote:
> > What do you think about storing decoded counter value in t8 in order to
> avoid...
> 
> t8 is not good for this, it's reserved for the macro-assembler. (we use it
> occasionally as short-lived temp, as in line 557). t2 seems available in this
> function, we could use that.

Done.

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:440: Label no_inobject_slack_tracking;
On 2014/05/23 15:33:39, Paul Lind wrote:
> On 2014/05/23 15:04:18, Igor Sheludko wrote:
> > On 2014/05/23 15:01:45, Paul Lind wrote:
> > > The deleted LoadRoot for UndefinedValue needs to be added above this if
> clause
> > -
> > > so  its available on all possible paths to  __
> InitializeFieldsWithFiller(t5,
> > > a0, t7)
> > 
> > There is one load at the top of the function. I think it is better to move
it
> > here, before the if().
> 
> I'd missed that one. I agree, better to move it here.

Done.

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:444: __ DecodeField<Map::ConstructionCount>(t0);
On 2014/05/23 15:33:39, Paul Lind wrote:
> On 2014/05/23 15:04:18, Igor Sheludko wrote:
> > ... reloading here, like it was done for ia32?
> 
> Sure, we can do that with t2. Balázs, please add comment about value in t2:
> slack tracking counter (non-API function case)

Done.

https://codereview.chromium.org/294973013/diff/1/src/mips/builtins-mips.cc#ne...
src/mips/builtins-mips.cc:453: __ Addu(t6, t4, Operand(at));   // End of object.
On 2014/05/23 15:04:18, Igor Sheludko wrote:
> I think we can now move above two lines inside the if() block below.

Done.

[Paul Lind, 2014-05-23 18:01:34]

lgtm

[Paul Lind, 2014-05-23 18:06:52]

Committed as r21470.

[Igor Sheludko, 2014-05-23 18:10:19]

Sorry for being late, but...

https://codereview.chromium.org/294973013/diff/40001/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/294973013/diff/40001/src/mips/builtins-mips.c...
src/mips/builtins-mips.cc:401: __ Pop(a1, a2);
t2 must be set to zero here (which is JSFunction::kNoSlackTracking) since the
slack tracking is actually over after the runtime call.

https://codereview.chromium.org/294973013/diff/40001/src/mips/builtins-mips.c...
src/mips/builtins-mips.cc:440: 
I think t7 initialization should be moved here:
  // Use t7 to hold undefined, which is used in several places below.
  __ LoadRoot(t7, Heap::kUndefinedValueRootIndex);

Not sure if it survives after the runtime call above.

[Paul Lind, 2014-05-23 18:38:22]

On 2014/05/23 18:10:19, Igor Sheludko wrote:
> t2 must be set to zero here (which is JSFunction::kNoSlackTracking) since the
> slack tracking is actually over after the runtime call.

> Not sure if t7 survives after the runtime call above.

Yep, t7 would get clobbered, and I not understand Runtime call changed slack
tracking count, so we'll clear t2.

New CL coming. Thanks again for your review.

[Paul Lind, 2014-05-23 18:46:35]

Of course t2 is just clobbered, and knowing runtime call sets count to 0 lets us
avoid reloading it.