Eliminate intentional conversion from Smi to Int32 in HMul

Eliminate intentional conversion from Smi to Int32 in HMul

If not all uses of arithmetic binary operation can be truncated to Smi, check if they can be truncated to Int32 which could avoid minus zero check

Fixed DoMulI on X64 to adopt correct operand size when the representation is Smi

Fixed DoMulI on ARM. Constant right operand optimization is based on Integer 32 instead of its representation.

BUG=
R=verwaest@chromium.org

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

[weiliang.lin2, 2013-08-08 15:02:17]

Elimination intentional conversion from Smi to Int32 in HMul could lead to
regress-2132.js failure on ia32. Through analysis, it is caused by a unnecessary
minus zero check.

[Sven Panne, 2013-08-09 05:56:23]

Test cases, please... :-)

[weiliang.lin2, 2013-08-09 06:00:14]

On 2013/08/09 05:56:23, Sven Panne wrote:
> Test cases, please... :-)

Hi Sven, 
test case is on this entry https://codereview.chromium.org/22573004/. Could you
please have a look

[Sven Panne, 2013-08-09 06:03:01]

On 2013/08/09 06:00:14, weiliang.lin2 wrote:
> On 2013/08/09 05:56:23, Sven Panne wrote:
> > Test cases, please... :-)
> 
> Hi Sven, 
> test case is on this entry https://codereview.chromium.org/22573004/. Could
you
> please have a look

Ah, OK, the CLs are related. I'll add Too a.k.a. "Mr. Smi" as a reviewer. ;-)

[weiliang.lin2, 2013-08-09 12:54:21]

On 2013/08/09 06:03:01, Sven Panne wrote:
> On 2013/08/09 06:00:14, weiliang.lin2 wrote:
> > On 2013/08/09 05:56:23, Sven Panne wrote:
> > > Test cases, please... :-)
> > 
> > Hi Sven, 
> > test case is on this entry https://codereview.chromium.org/22573004/. Could
> you
> > please have a look
> 
> Ah, OK, the CLs are related. I'll add Too a.k.a. "Mr. Smi" as a reviewer. ;-)

Hi Toon, 
Could  you please have a review of this commit and then another one
(https://codereview.chromium.org/22573004/). Thanks

[Toon Verwaest, 2013-08-09 14:10:13]

One comment so far, but could you please merge both HMul/smi related issues
together? They are entirely dependent on each other; so I'd like to review them
together in one CL.

https://chromiumcodereview.appspot.com/22600005/diff/1/src/hydrogen-canonical...
File src/hydrogen-canonicalize.cc (right):

https://chromiumcodereview.appspot.com/22600005/diff/1/src/hydrogen-canonical...
src/hydrogen-canonicalize.cc:50:
instr->SetFlag(HInstruction::kAllUsesTruncatingToSmi);
We should probably always set TruncatingToInt32 everywhere where we set
TruncatingToSmi, to ensure we at least have TruncatingToInt32.

[weiliang.lin2, 2013-08-09 15:09:50]

HMul/smi related issues are merged here. 
Now, ia32/x64 are done. For ARM platform, there are still some failures, WIP.
Thanks for review

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

https://codereview.chromium.org/22600005/diff/1/src/hydrogen-canonicalize.cc#...
src/hydrogen-canonicalize.cc:50:
instr->SetFlag(HInstruction::kAllUsesTruncatingToSmi);
On 2013/08/09 14:10:13, Toon Verwaest wrote:
> We should probably always set TruncatingToInt32 everywhere where we set
> TruncatingToSmi, to ensure we at least have TruncatingToInt32.

Done.

[weiliang.lin2, 2013-08-10 13:51:26]

Hi Toon
The CL is ready. Please review it. Thanks

[Toon Verwaest, 2013-08-10 14:13:19]

Looking good, but we should fix up the algorithm that annotates phis with
truncation information.

https://codereview.chromium.org/22600005/diff/15001/src/hydrogen-representati...
File src/hydrogen-representation-changes.cc (right):

https://codereview.chromium.org/22600005/diff/15001/src/hydrogen-representati...
src/hydrogen-representation-changes.cc:131: !(input_representation.IsSmi() ||
What about just cleaning up the code (and making the algorithm accurate) by
splitting it into separate worklists for smi and int32?

if (phi->representation().IsSmiOrInteger32() &&
    !phi->CheckUsesForFlag(HValue::kTruncatingToInt32)) {
  int_worklist.Add(phi, zone());
  phi->ClearFlag(HValue::kTruncatingToInt32);
}

if (phi->representation().IsSmi() &&
    !phi->CheckUsesForFlag(HValue::kTruncatingToSmi)) {
  smi_worklist.Add(phi, zone());
  phi->ClearFlag(HValue::kTruncatingToSmi);
}

And then processing the two worklists individually.

https://codereview.chromium.org/22600005/diff/15001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/22600005/diff/15001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1381: // Constant can't be represented as Smi due
to immediate size limit
Full stop at the end of the sentence.

[weiliang.lin2, 2013-08-10 16:43:26]

Yes, that algorithm should be modified based on your comments. Thanks a lot~

https://codereview.chromium.org/22600005/diff/15001/src/hydrogen-representati...
File src/hydrogen-representation-changes.cc (right):

https://codereview.chromium.org/22600005/diff/15001/src/hydrogen-representati...
src/hydrogen-representation-changes.cc:131: !(input_representation.IsSmi() ||
On 2013/08/10 14:13:19, Toon Verwaest wrote:
> What about just cleaning up the code (and making the algorithm accurate) by
> splitting it into separate worklists for smi and int32?
> 
> if (phi->representation().IsSmiOrInteger32() &&
>     !phi->CheckUsesForFlag(HValue::kTruncatingToInt32)) {
>   int_worklist.Add(phi, zone());
>   phi->ClearFlag(HValue::kTruncatingToInt32);
> }
> 
> if (phi->representation().IsSmi() &&
>     !phi->CheckUsesForFlag(HValue::kTruncatingToSmi)) {
>   smi_worklist.Add(phi, zone());
>   phi->ClearFlag(HValue::kTruncatingToSmi);
> }
> 
> And then processing the two worklists individually.

Done. And to keep trace_representation, add another CheckUsesForFlag function to
return the Instruction without the flag.

https://codereview.chromium.org/22600005/diff/15001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/22600005/diff/15001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1381: // Constant can't be represented as Smi due
to immediate size limit
On 2013/08/10 14:13:19, Toon Verwaest wrote:
> Full stop at the end of the sentence.

Done.

[Toon Verwaest, 2013-08-11 16:11:49]

Octane's navier stokes benchmark fails with the current implementation. Enabling
smi for HMul also seems to tank octane's crypto by ~7%; bitops bits in byte by
quite a bit; and also audio-fft of kraken seems to go down on i5. Is there
something we can do about that? The overall impact isn't look too negative, but
we definitely have to fix navier stokes.

[weiliang.lin2, 2013-08-12 06:24:15]

On 2013/08/11 16:11:49, Toon Verwaest wrote:
> Octane's navier stokes benchmark fails with the current implementation.
Enabling
> smi for HMul also seems to tank octane's crypto by ~7%; bitops bits in byte by
> quite a bit; and also audio-fft of kraken seems to go down on i5. Is there
> something we can do about that? The overall impact isn't look too negative,
but
> we definitely have to fix navier stokes.

Fixed the navier stokes benchmark fails. 

Current the general benchmark performance on IA32 and X64 is as follows:
IA32         Octane      SunSpider      Kraken
Origin       15637       219.7          1796.6
HMul/SMI     15610       219.5          1804.3

X64          Octane      SunSpider      Kraken
Origin       14935       217.9          1745.7
HMul/SMI     14917       217.8          1741.8

I don't find the 7% regression of crypto benchmark. audio-fft seems to go a bit
down on ia32 and go a bit up on X64.

[weiliang.lin2, 2013-08-12 14:36:45]

On 2013/08/12 06:24:15, weiliang.lin2 wrote:
> On 2013/08/11 16:11:49, Toon Verwaest wrote:
> > Octane's navier stokes benchmark fails with the current implementation.
> Enabling
> > smi for HMul also seems to tank octane's crypto by ~7%; bitops bits in byte
by
> > quite a bit; and also audio-fft of kraken seems to go down on i5. Is there
> > something we can do about that? The overall impact isn't look too negative,
> but
> > we definitely have to fix navier stokes.
> 
> Fixed the navier stokes benchmark fails. 
> 
> Current the general benchmark performance on IA32 and X64 is as follows:
> IA32         Octane      SunSpider      Kraken
> Origin       15637       219.7          1796.6
> HMul/SMI     15610       219.5          1804.3
> 
> X64          Octane      SunSpider      Kraken
> Origin       14935       217.9          1745.7
> HMul/SMI     14917       217.8          1741.8
> 
> I don't find the 7% regression of crypto benchmark. audio-fft seems to go a
bit
> down on ia32 and go a bit up on X64.


One finding till now, https://codereview.chromium.org/22824003/

[weiliang.lin2, 2013-08-12 14:41:00]

On 2013/08/12 14:36:45, weiliang.lin2 wrote:
> On 2013/08/12 06:24:15, weiliang.lin2 wrote:
> > On 2013/08/11 16:11:49, Toon Verwaest wrote:
> > > Octane's navier stokes benchmark fails with the current implementation.
> > Enabling
> > > smi for HMul also seems to tank octane's crypto by ~7%; bitops bits in
byte
> by
> > > quite a bit; and also audio-fft of kraken seems to go down on i5. Is there
> > > something we can do about that? The overall impact isn't look too
negative,
> > but
> > > we definitely have to fix navier stokes.
> > 
> > Fixed the navier stokes benchmark fails. 
> > 
> > Current the general benchmark performance on IA32 and X64 is as follows:
> > IA32         Octane      SunSpider      Kraken
> > Origin       15637       219.7          1796.6
> > HMul/SMI     15610       219.5          1804.3
> > 
> > X64          Octane      SunSpider      Kraken
> > Origin       14935       217.9          1745.7
> > HMul/SMI     14917       217.8          1741.8
> > 
> > I don't find the 7% regression of crypto benchmark. audio-fft seems to go a
> bit
> > down on ia32 and go a bit up on X64.
> 
> 
> One finding till now, https://codereview.chromium.org/22824003/

Forget to mention that this could solve the audio-fft slow down issue.

[Toon Verwaest, 2013-08-12 17:51:11]

Lgtm, thanks. I'll land it when the tree is open again.

[Toon Verwaest, 2013-08-27 13:55:16]

Committed patchset #6 manually as r16361 (presubmit successful).