Add initial hydrogenized NewStringAddStub.

Add initial hydrogenized NewStringAddStub.

The new stub is enabled via the --new-string-add flag, which is
disabled by default. For now, it's only a stripped down version
of the native StringAddStub, it's still work-in-progress.

BUG=v8:2990
R=mvstanton@chromium.org

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

[Benedikt Meurer, 2013-11-07 14:05:25]

Hey Michael,
As promised, here's the initial NewStringAddStub. :-)
PTAL
-- Benedikt

[mvstanton, 2013-11-08 10:17:49]

Hi Benedikt, here are comments. Looking good!
--Michael

https://codereview.chromium.org/61893009/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/61893009/diff/1/src/code-stubs.cc#newcode684
src/code-stubs.cc:684: void NewStringAddStub::PrintBaseName(StringStream*
stream) {
Wow, I'm going to add this to some of my own stubs, very nice. :)

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1495
src/hydrogen.cc:1495: // right now.
I appreciate the detailed comments on this long and tricky method! 
Instead of saying "this may be too pessimistic," could you say what you would
prefer to do, and then lament that it's non-portable right now.

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1558
src/hydrogen.cc:1558:
Add<HConstant>(static_cast<int32_t>(kOneByteDataHintMask))),
These hurt my eyes. :)
How about a helper function that consolidates this kind of typical bitwise
operation for this method. The load of Add, <, >, static_cast, obscures the
overall operation.

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1612
src/hydrogen.cc:1612:
Add<HConstant>(static_cast<int32_t>(kStringEncodingMask))),
More instances of your typical pattern here, OP1(OP2(hvalue1, hvalue2), mask).

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1647
src/hydrogen.cc:1647: 
The size operand calculation above is very tricky and very similar to the block
below (under if_onebyte.Else()). Can it be moved into a function?

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1655
src/hydrogen.cc:1655: HStoreNamedField* store_map = AddStoreMapConstant(string,
map);
AddStoreMapConstantNoWriteBarrier() again, or as a mode of
AddStoreMapConstant().

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1698
src/hydrogen.cc:1698: HStoreNamedField* store_map = AddStoreMapConstant(string,
map);
Could you add a variant to AddStoreMapConstant either with a parameter or a new
name (like AddStoreMapConstantNoWriteBarrier) that encapsulates these? As
discussed earlier, this "special case" of skipping write barriers can be
dangerous to program, and it would be nice to consolidate and better name those
instances when we do this.

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

https://codereview.chromium.org/61893009/diff/1/src/ia32/full-codegen-ia32.cc...
src/ia32/full-codegen-ia32.cc:3679: NewStringAddStub stub(STRING_ADD_CHECK_BOTH,
NOT_TENURED);
Is there a loss of performance here because we don't pretenure according to the
global mode from full codegen any more? As we move to site-based pretenuring,
will you have a version of NewStringAddStub that creates mementos?

[Benedikt Meurer, 2013-11-08 13:21:07]

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1495
src/hydrogen.cc:1495: // right now.
On 2013/11/08 10:17:50, mvstanton wrote:
> I appreciate the detailed comments on this long and tricky method! 
> Instead of saying "this may be too pessimistic," could you say what you would
> prefer to do, and then lament that it's non-portable right now.

Done.

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1558
src/hydrogen.cc:1558:
Add<HConstant>(static_cast<int32_t>(kOneByteDataHintMask))),
On 2013/11/08 10:17:50, mvstanton wrote:
> These hurt my eyes. :)
> How about a helper function that consolidates this kind of typical bitwise
> operation for this method. The load of Add, <, >, static_cast, obscures the
> overall operation.

I'm not sure that's going to make it better. Tried that already, but it's even
less obvious what's happening then, IMHO.

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1647
src/hydrogen.cc:1647: 
On 2013/11/08 10:17:50, mvstanton wrote:
> The size operand calculation above is very tricky and very similar to the
block
> below (under if_onebyte.Else()). Can it be moved into a function?

Done.

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1655
src/hydrogen.cc:1655: HStoreNamedField* store_map = AddStoreMapConstant(string,
map);
On 2013/11/08 10:17:50, mvstanton wrote:
> AddStoreMapConstantNoWriteBarrier() again, or as a mode of
> AddStoreMapConstant().

Done.

https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1698
src/hydrogen.cc:1698: HStoreNamedField* store_map = AddStoreMapConstant(string,
map);
On 2013/11/08 10:17:50, mvstanton wrote:
> Could you add a variant to AddStoreMapConstant either with a parameter or a
new
> name (like AddStoreMapConstantNoWriteBarrier) that encapsulates these? As
> discussed earlier, this "special case" of skipping write barriers can be
> dangerous to program, and it would be nice to consolidate and better name
those
> instances when we do this.

Done.

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

https://codereview.chromium.org/61893009/diff/1/src/ia32/full-codegen-ia32.cc...
src/ia32/full-codegen-ia32.cc:3679: NewStringAddStub stub(STRING_ADD_CHECK_BOTH,
NOT_TENURED);
On 2013/11/08 10:17:50, mvstanton wrote:
> Is there a loss of performance here because we don't pretenure according to
the
> global mode from full codegen any more? As we move to site-based pretenuring,
> will you have a version of NewStringAddStub that creates mementos?

Yes.

[mvstanton, 2013-11-11 10:17:20]

Right on, lgtm.

[Benedikt Meurer, 2013-11-12 10:21:24]

Committed patchset #4 manually as r17635 (presubmit successful).