Generated code for substring slices in ia32.

Generated code for substring slices in ia32.

Committed: http://code.google.com/p/v8/source/detail?r=9064

[Yang, 2011-08-26 15:58:39]

Please take a look.

[antonm, 2011-08-26 16:10:55]

LGTM

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5707: __ SmiTag(edx);  // Make edx a smi again.
do you want to fallback here from make_two_character_string?

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5728: const uint32_t kSlicedNotConsMask =
kSlicedStringTag & ~kConsStringTag;
lift those constants into objects.h?

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5747: __ mov(edi, eax);
maybe keep the parent string in eax and save this move?

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5769: __ SmiTag(ecx);
apparently there is too much of tagging/untagging, but it should have only
minimal perf impact.

[Yang, 2011-08-29 12:55:56]

Maybe Vitaly wants to have a look as well?

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5707: __ SmiTag(edx);  // Make edx a smi again.
On 2011/08/26 16:10:55, antonm wrote:
> do you want to fallback here from make_two_character_string?

Yes.

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5747: __ mov(edi, eax);
On 2011/08/26 16:10:55, antonm wrote:
> maybe keep the parent string in eax and save this move?

eax has to be used as destination when allocation the new sliced string and as
return value.

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5769: __ SmiTag(ecx);
On 2011/08/26 16:10:55, antonm wrote:
> apparently there is too much of tagging/untagging, but it should have only
> minimal perf impact.

The alternative saves this one tagging, but would make may other places ugly
(compare with shifted immediates etc.). The reason is that many macros take
un-smi-tagged register values as parameter.

[antonm, 2011-08-29 14:51:27]

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5707: __ SmiTag(edx);  // Make edx a smi again.
On 2011/08/29 12:55:56, Yang wrote:
> On 2011/08/26 16:10:55, antonm wrote:
> > do you want to fallback here from make_two_character_string?
> 
> Yes.

Brevity is .... :)  I meant you know for sure you won't need a slice for two
char string, so you're doing redundant work.

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5728: const uint32_t kSlicedNotConsMask =
kSlicedStringTag & ~kConsStringTag;
On 2011/08/26 16:10:55, antonm wrote:
> lift those constants into objects.h?

any response?

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5747: __ mov(edi, eax);
On 2011/08/29 12:55:56, Yang wrote:
> On 2011/08/26 16:10:55, antonm wrote:
> > maybe keep the parent string in eax and save this move?
> 
> eax has to be used as destination when allocation the new sliced string and as
> return value.

I am not sure, apparently you can allocate into any register you like, at least
w/ some minimal amount of work (at least AllocateInNewSpace behaves this way)

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5769: __ SmiTag(ecx);
On 2011/08/29 12:55:56, Yang wrote:
> On 2011/08/26 16:10:55, antonm wrote:
> > apparently there is too much of tagging/untagging, but it should have only
> > minimal perf impact.
> 
> The alternative saves this one tagging, but would make may other places ugly
> (compare with shifted immediates etc.). The reason is that many macros take
> un-smi-tagged register values as parameter.

Up to you.

[Yang, 2011-08-29 15:17:07]

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5707: __ SmiTag(edx);  // Make edx a smi again.
On 2011/08/29 14:51:27, antonm wrote:
> On 2011/08/29 12:55:56, Yang wrote:
> > On 2011/08/26 16:10:55, antonm wrote:
> > > do you want to fallback here from make_two_character_string?
> > 
> > Yes.
> 
> Brevity is .... :)  I meant you know for sure you won't need a slice for two
> char string, so you're doing redundant work.

Oups. I actually managed to overlook this simple fact. Thanks.

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5728: const uint32_t kSlicedNotConsMask =
kSlicedStringTag & ~kConsStringTag;
On 2011/08/29 14:51:27, antonm wrote:
> On 2011/08/26 16:10:55, antonm wrote:
> > lift those constants into objects.h?
> 
> any response?

I did move these two lines into objects.h
Maybe you were looking in the wrong diff.

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5747: __ mov(edi, eax);
On 2011/08/29 14:51:27, antonm wrote:
> On 2011/08/29 12:55:56, Yang wrote:
> > On 2011/08/26 16:10:55, antonm wrote:
> > > maybe keep the parent string in eax and save this move?
> > 
> > eax has to be used as destination when allocation the new sliced string and
as
> > return value.
> 
> I am not sure, apparently you can allocate into any register you like, at
least
> w/ some minimal amount of work (at least AllocateInNewSpace behaves this way)

Yes but the issue is that the return value of this code stub is fixed to be eax.
Now I could of course load the original string into edi to begin with, but then
I would have to move edi into eax in case the substring is the same as the
original string.
So the issue is that the return value has to be eax, the original string has to
be loaded into eax as well since we need to be able to return the original
string immediately if  necessary.

[antonm, 2011-08-29 17:19:26]

Still LGTM, but feel free to wait for Vitlay's review as well.

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5728: const uint32_t kSlicedNotConsMask =
kSlicedStringTag & ~kConsStringTag;
On 2011/08/29 15:17:07, Yang wrote:
> On 2011/08/29 14:51:27, antonm wrote:
> > On 2011/08/26 16:10:55, antonm wrote:
> > > lift those constants into objects.h?
> > 
> > any response?
> 
> I did move these two lines into objects.h
> Maybe you were looking in the wrong diff.

Sorry, didn't notice that.  Usually people respond w/ something like Done.

http://codereview.chromium.org/7744052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5747: __ mov(edi, eax);
On 2011/08/29 15:17:07, Yang wrote:
> On 2011/08/29 14:51:27, antonm wrote:
> > On 2011/08/29 12:55:56, Yang wrote:
> > > On 2011/08/26 16:10:55, antonm wrote:
> > > > maybe keep the parent string in eax and save this move?
> > > 
> > > eax has to be used as destination when allocation the new sliced string
and
> as
> > > return value.
> > 
> > I am not sure, apparently you can allocate into any register you like, at
> least
> > w/ some minimal amount of work (at least AllocateInNewSpace behaves this
way)
> 
> Yes but the issue is that the return value of this code stub is fixed to be
eax.
> Now I could of course load the original string into edi to begin with, but
then
> I would have to move edi into eax in case the substring is the same as the
> original string.
> So the issue is that the return value has to be eax, the original string has
to
> be loaded into eax as well since we need to be able to return the original
> string immediately if  necessary.

ok

http://codereview.chromium.org/7744052/diff/9001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7744052/diff/9001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:5725: STATIC_ASSERT(kIsIndirectStringMask ==
(kSlicedStringTag & kConsStringTag));
may those go too?

[Vitaly Repeshko, 2011-08-30 01:17:47]

LGTM

http://codereview.chromium.org/7744052/diff/9001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7744052/diff/9001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:5756: // Allocate new sliced string.  At this point
we do not reload the instance
This is clever, but the comment is a bit confusing. I'd replace "parent" with
"original" to match the comment about ebx. Also later it may help to use "newly
created slice" to avoid confusion with the one we might have just unwrapped.