Improve the assembly code for power function with integer exponential.

Improve the assembly code for power function with integer exponential. 

The change removes one unused multiply and reschedules
the shift, multiply and jump instructions to reduce
stall. Experiment shows it improve about 20% performance
on x86 for exponetials from about 100 to 2000.

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

[xqian, 2012-09-14 14:36:44]

Hi,Yang, Kasper and Yuqiang

I have a patch for intrinsic power function on ia32. Would you please help to
review it?

Thanks,
-Xi

[Yang, 2012-09-14 15:38:16]

Thanks for this patch! I'll land this patch once the following two comments are
addressed.

https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:3224: __ j(above, &while_true, Label::kNear);
Please add a comment here that the above flag is set if the bit shifted out was
not set. This would make understanding this easier.

https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:3225: __ mulsd(double_result, double_scratch);
Since double_result is 1 here, wouldn't movq(double_result, double_scratch) do
the same? That would actually be more readable imo. Maybe also faster?

[xqian, 2012-09-17 03:10:48]

Hi， Yang

Thanks for your review. I refined the code according to your comments. Please
check the patch set 3. 

Thanks,
-Xi
On 2012/09/14 15:38:16, Yang wrote:
> Thanks for this patch! I'll land this patch once the following two comments
are
> addressed.
> 
> https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.cc
> File src/ia32/code-stubs-ia32.cc (right):
> 
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> src/ia32/code-stubs-ia32.cc:3224: __ j(above, &while_true, Label::kNear);
> Please add a comment here that the above flag is set if the bit shifted out
was
> not set. This would make understanding this easier.
> 
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> src/ia32/code-stubs-ia32.cc:3225: __ mulsd(double_result, double_scratch);
> Since double_result is 1 here, wouldn't movq(double_result, double_scratch) do
> the same? That would actually be more readable imo. Maybe also faster?

[Yang, 2012-09-17 07:40:14]

On 2012/09/17 03:10:48, xqian wrote:
> Hi， Yang
> 
> Thanks for your review. I refined the code according to your comments. Please
> check the patch set 3. 
> 
> Thanks,
> -Xi
> On 2012/09/14 15:38:16, Yang wrote:
> > Thanks for this patch! I'll land this patch once the following two comments
> are
> > addressed.
> > 
> >
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.cc
> > File src/ia32/code-stubs-ia32.cc (right):
> > 
> >
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> > src/ia32/code-stubs-ia32.cc:3224: __ j(above, &while_true, Label::kNear);
> > Please add a comment here that the above flag is set if the bit shifted out
> was
> > not set. This would make understanding this easier.
> > 
> >
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> > src/ia32/code-stubs-ia32.cc:3225: __ mulsd(double_result, double_scratch);
> > Since double_result is 1 here, wouldn't movq(double_result, double_scratch)
do
> > the same? That would actually be more readable imo. Maybe also faster?

LGTM. Landing.

Does it make sense to port this to x64?

[xqian, 2012-09-17 09:39:15]

On 2012/09/17 07:40:14, Yang wrote:
> On 2012/09/17 03:10:48, xqian wrote:
> > Hi， Yang
> > 
> > Thanks for your review. I refined the code according to your comments.
Please
> > check the patch set 3. 
> > 
> > Thanks,
> > -Xi
> > On 2012/09/14 15:38:16, Yang wrote:
> > > Thanks for this patch! I'll land this patch once the following two
comments
> > are
> > > addressed.
> > > 
> > >
> https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.cc
> > > File src/ia32/code-stubs-ia32.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> > > src/ia32/code-stubs-ia32.cc:3224: __ j(above, &while_true, Label::kNear);
> > > Please add a comment here that the above flag is set if the bit shifted
out
> > was
> > > not set. This would make understanding this easier.
> > > 
> > >
> >
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> > > src/ia32/code-stubs-ia32.cc:3225: __ mulsd(double_result, double_scratch);
> > > Since double_result is 1 here, wouldn't movq(double_result,
double_scratch)
> do
> > > the same? That would actually be more readable imo. Maybe also faster?
> 
> LGTM. Landing.
> 
> Does it make sense to port this to x64?

The x64 follows the same algorithm for integer exponential. It should be worth
to try. I'll verify the performance. 

-Xi

[Vyacheslav Egorov (Google), 2012-09-17 22:12:02]

DBC

https://codereview.chromium.org/10916311/diff/5002/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/10916311/diff/5002/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:3216: Label no_neg, while_true, no_multiply,
while_false;
I have an impression that no_multiply is not used anymore.

[xqian, 2012-09-18 06:29:57]

On 2012/09/17 22:12:02, Vyacheslav Egorov (Google) wrote:
> DBC
> 
> https://codereview.chromium.org/10916311/diff/5002/src/ia32/code-stubs-ia32.cc
> File src/ia32/code-stubs-ia32.cc (right):
> 
>
https://codereview.chromium.org/10916311/diff/5002/src/ia32/code-stubs-ia32.c...
> src/ia32/code-stubs-ia32.cc:3216: Label no_neg, while_true, no_multiply,
> while_false;
> I have an impression that no_multiply is not used anymore.

Thank you for pointing this out. Yes, no_multiply is not used anymore.    I'm
not sure if I can still change it now. 

BTW: what does DBC mean?

Thanks,
-Xi

[xqian, 2012-09-18 06:36:25]

On 2012/09/17 07:40:14, Yang wrote:
> On 2012/09/17 03:10:48, xqian wrote:
> > Hi， Yang
> > 
> > Thanks for your review. I refined the code according to your comments.
Please
> > check the patch set 3. 
> > 
> > Thanks,
> > -Xi
> > On 2012/09/14 15:38:16, Yang wrote:
> > > Thanks for this patch! I'll land this patch once the following two
comments
> > are
> > > addressed.
> > > 
> > >
> https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.cc
> > > File src/ia32/code-stubs-ia32.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> > > src/ia32/code-stubs-ia32.cc:3224: __ j(above, &while_true, Label::kNear);
> > > Please add a comment here that the above flag is set if the bit shifted
out
> > was
> > > not set. This would make understanding this easier.
> > > 
> > >
> >
>
https://codereview.chromium.org/10916311/diff/3001/src/ia32/code-stubs-ia32.c...
> > > src/ia32/code-stubs-ia32.cc:3225: __ mulsd(double_result, double_scratch);
> > > Since double_result is 1 here, wouldn't movq(double_result,
double_scratch)
> do
> > > the same? That would actually be more readable imo. Maybe also faster?
> 
> LGTM. Landing.
> 
> Does it make sense to port this to x64?

I tried the patch on x64 and it got similar performance improvement. The patch
is created at https://codereview.chromium.org/10939013/. Would you please review
it?

Thanks,
-Xi