Use xorps to break the cvtsi2sd unnecessary dependence due to its partially written

Use xorps to break the cvtsi2sd unnecessary dependence due to its partially written

BUG=
R=yangguo@chromium.org

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

[weiliang.lin2, 2013-09-10 14:39:13]

There is obvious performance improvement on IA32 when using xorps to solve the
cvtsi2sd partially written issue. The most benefit case is the audio-oscillator
of kraken, which is improved by more than 15% and there is no negative impact on
other cases. And for octane, It seems overall there is also slight improvement
(about 1%, measure 3 times to calculate its average value) on my x86 PC.

[Jakob Kummerow, 2013-09-11 11:34:14]

Redirecting to Yang.

[Yang, 2013-09-11 13:37:02]

On 2013/09/11 11:34:14, Jakob wrote:
> Redirecting to Yang.

We use cvtsi2sd in a couple of other places as well. Would it make sense to do
that elsewhere as well? Or even bundle xorps and cvtsi2sd into a MacroAssembler
method and use that everywhere?

[Sven Panne, 2013-09-11 13:43:06]

Could you describe the underlying reason why zeroing out things first makes this
faster? The upper half remained unchanged before. It would be good to know the
actual pattern of instructions to avoid.

[weiliang.lin2, 2013-09-12 01:44:43]

On 2013/09/11 13:37:02, Yang wrote:
> On 2013/09/11 11:34:14, Jakob wrote:
> > Redirecting to Yang.
> 
> We use cvtsi2sd in a couple of other places as well. Would it make sense to do
> that elsewhere as well? Or even bundle xorps and cvtsi2sd into a
MacroAssembler
> method and use that everywhere?

Sound good suggestion. But I need to measure more performance test to see
whether using it everywhere has negative impact. 

Actually, I found this issue when I tune this fix
(https://codereview.chromium.org/23717039/). I found that when disable
left/right switch has big negative performance impact on audio-oscillator case.
From ll-prof, there is quite big latency for cvtsi2sd instruction (emitted by
Integer32ToInt). 

So I tried to use the common way to break its false dependency. 

@Sven
Please see http://www.agner.org/optimize/optimizing_assembly.pdf Section 9.5
Break dependency chains

[weiliang.lin2, 2013-09-12 08:06:51]

bundle xorps and cvtsi2sd into a MacroAssembler. Currently use it everywhere.
There is not any performance regression compared with patch set 1 and has the
same benefit.

Currently This bundle is used everywhere temporary. If we find there is any
performance issue on some scenarios, we can change it back. 

I will port it to X64 backend.

[weiliang.lin2, 2013-09-12 08:51:03]

x64 port done

[Yang, 2013-09-12 11:56:49]

I can confirm the performance improvements. This is great!

Could you add a comment on the background of this change? From what I gathered,
this has something to do with register renaming internal to the CPU?

Also, could you rebase before I land this change? Some of the calls of cvtsi2sd
have been removed yesterday.

https://codereview.chromium.org/23654026/diff/7001/src/ia32/macro-assembler-i...
File src/ia32/macro-assembler-ia32.h (right):

https://codereview.chromium.org/23654026/diff/7001/src/ia32/macro-assembler-i...
src/ia32/macro-assembler-ia32.h:369: // Use xorps to clear out the destination
register before cvtsi2sd
Could you add a comment on why we do this? I mean, it's not entirely obvious why
we need to clear out the destination register, so it would be great if somebody
working on this code in the future can figure out what the background of this
change is.

[weiliang.lin2, 2013-09-12 14:15:03]

On 2013/09/12 11:56:49, Yang wrote:
> I can confirm the performance improvements. This is great!
> 
> Could you add a comment on the background of this change? From what I
gathered,
> this has something to do with register renaming internal to the CPU?
> 
> Also, could you rebase before I land this change? Some of the calls of
cvtsi2sd
> have been removed yesterday.
> 
>
https://codereview.chromium.org/23654026/diff/7001/src/ia32/macro-assembler-i...
> File src/ia32/macro-assembler-ia32.h (right):
> 
>
https://codereview.chromium.org/23654026/diff/7001/src/ia32/macro-assembler-i...
> src/ia32/macro-assembler-ia32.h:369: // Use xorps to clear out the destination
> register before cvtsi2sd
> Could you add a comment on why we do this? I mean, it's not entirely obvious
why
> we need to clear out the destination register, so it would be great if
somebody
> working on this code in the future can figure out what the background of this
> change is.

Thanks for review. I have rebased it to master and could you please have a
review?

[Yang, 2013-09-12 15:28:57]

On 2013/09/12 14:15:03, weiliang.lin2 wrote:
> On 2013/09/12 11:56:49, Yang wrote:
> > I can confirm the performance improvements. This is great!
> > 
> > Could you add a comment on the background of this change? From what I
> gathered,
> > this has something to do with register renaming internal to the CPU?
> > 
> > Also, could you rebase before I land this change? Some of the calls of
> cvtsi2sd
> > have been removed yesterday.
> > 
> >
>
https://codereview.chromium.org/23654026/diff/7001/src/ia32/macro-assembler-i...
> > File src/ia32/macro-assembler-ia32.h (right):
> > 
> >
>
https://codereview.chromium.org/23654026/diff/7001/src/ia32/macro-assembler-i...
> > src/ia32/macro-assembler-ia32.h:369: // Use xorps to clear out the
destination
> > register before cvtsi2sd
> > Could you add a comment on why we do this? I mean, it's not entirely obvious
> why
> > we need to clear out the destination register, so it would be great if
> somebody
> > working on this code in the future can figure out what the background of
this
> > change is.
> 
> Thanks for review. I have rebased it to master and could you please have a
> review?

LGTM. I'll land this.

[Yang, 2013-09-13 08:00:02]

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

[Sven Panne, 2013-10-16 08:58:17]

[ Reviving the dead... :-]
While the performance improvement on kraken's audio-oscillator is great, I've
just seen that we actually regress SunSpider's math-spectral-norm by roughly 25%
on linux-x64 (Core 2 and Core i5). I am not sure what the actual reason is,
bmeurer@ has the theory that it might be related to single vs. double mode.
Anyway, we might have to investigate this further, any help from the Intel side
would be highly appreciated.

[Weiliang, 2013-10-17 04:02:14]

On 2013/10/16 08:58:17, Sven Panne wrote:
> [ Reviving the dead... :-]
> While the performance improvement on kraken's audio-oscillator is great, I've
> just seen that we actually regress SunSpider's math-spectral-norm by roughly
25%
> on linux-x64 (Core 2 and Core i5). I am not sure what the actual reason is,
> bmeurer@ has the theory that it might be related to single vs. double mode.
> Anyway, we might have to investigate this further, any help from the Intel
side
> would be highly appreciated.

Hi Sven, 
Thanks a lot for your sharing. 

I did a quick experiment on my PC (Core 2) by simply not using xorps before
cvtsi2sd as below:

diff --git a/src/x64/macro-assembler-x64.cc b/src/x64/macro-assembler-x64.cc
index 9dcb9d1..05ae973 100644
--- a/src/x64/macro-assembler-x64.cc
+++ b/src/x64/macro-assembler-x64.cc
@@ -936,13 +936,11 @@ void MacroAssembler::PopCallerSaved(SaveFPRegsMode
fp_mode,


 void MacroAssembler::Cvtlsi2sd(XMMRegister dst, Register src) {
-  xorps(dst, dst);
   cvtlsi2sd(dst, src);
 }


 void MacroAssembler::Cvtlsi2sd(XMMRegister dst, const Operand& src) {
-  xorps(dst, dst);
   cvtlsi2sd(dst, src);
 }



Then I got the following result for SunSpider's math-spectral-norm. Because
math-spectral-norm is a quite small case (only 4-5 ms in my machine), I load it
ten time to measure its performance.

Case: (run.js)
var _start = new Date();
for (var ii = 0; ii < 10; ii += 1) {
    load("math-spectral-norm.js");
}
var elapsed = new Date() - _start;
print(elapsed + " ms");

Result:
[~]./d8.x64.no.xorps run.js
28 ms
[~]./d8.x64.xorps run.js
23 ms

So it seems there is ~17% improvement by using xorps. :)