Small improvement in x64 assembler

Small improvement in x64 assembler

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

[Zheng Liu, 2013-02-04 11:32:47]

PTAL

I'm not sure if param src of LoadUint32() is guaranteed to have upper half
zeroed?

[Yang, 2013-02-06 10:28:59]

Comments attached.

https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc#n...
src/x64/assembler-x64.cc:1511: } else if (is_int32(value)) {
What is the purpose of this change?

https://codereview.chromium.org/12177012/diff/3001/src/x64/macro-assembler-x6...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/12177012/diff/3001/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.cc:2921: cvtqsi2sd(dst, src);
The uses of this indicate that the input is indeed uint32, so I think thet
assert is sufficient.

[Zheng Liu, 2013-02-06 11:34:08]

https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc#n...
src/x64/assembler-x64.cc:1511: } else if (is_int32(value)) {
On 2013/02/06 10:28:59, Yang wrote:
> What is the purpose of this change?

Code might be smaller, as [mov r64, imm32] always have a prefix.
MacroAssembler::Set() did similar things.

[Yang, 2013-02-06 11:39:18]

https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc#n...
src/x64/assembler-x64.cc:1511: } else if (is_int32(value)) {
On 2013/02/06 11:34:08, Zheng Liu wrote:
> On 2013/02/06 10:28:59, Yang wrote:
> > What is the purpose of this change?
> 
> Code might be smaller, as [mov r64, imm32] always have a prefix.
> MacroAssembler::Set() did similar things.

But this change seems to be to the effect of changing

if (A) {
  B;
} else if (C) {
  D;
}

into

if (C) {
  D;
} else if (A) {
  B;
}

I don't see any difference besides changing the order.

[Zheng Liu, 2013-02-06 11:44:41]

On 2013/02/06 11:39:18, Yang wrote:
> https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc
> File src/x64/assembler-x64.cc (right):
> 
>
https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc#n...
> src/x64/assembler-x64.cc:1511: } else if (is_int32(value)) {
> On 2013/02/06 11:34:08, Zheng Liu wrote:
> > On 2013/02/06 10:28:59, Yang wrote:
> > > What is the purpose of this change?
> > 
> > Code might be smaller, as [mov r64, imm32] always have a prefix.
> > MacroAssembler::Set() did similar things.
> 
> But this change seems to be to the effect of changing
> 
> if (A) {
>   B;
> } else if (C) {
>   D;
> }
> 
> into
> 
> if (C) {
>   D;
> } else if (A) {
>   B;
> }
> 
> I don't see any difference besides changing the order.

I think that cond A & C overlaps, e.g. is_uint32(0x1) and is_int32(0x1) are both
true.

[Yang, 2013-02-06 11:46:58]

On 2013/02/06 11:44:41, Zheng Liu wrote:
> On 2013/02/06 11:39:18, Yang wrote:
> > https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc
> > File src/x64/assembler-x64.cc (right):
> > 
> >
>
https://codereview.chromium.org/12177012/diff/3001/src/x64/assembler-x64.cc#n...
> > src/x64/assembler-x64.cc:1511: } else if (is_int32(value)) {
> > On 2013/02/06 11:34:08, Zheng Liu wrote:
> > > On 2013/02/06 10:28:59, Yang wrote:
> > > > What is the purpose of this change?
> > > 
> > > Code might be smaller, as [mov r64, imm32] always have a prefix.
> > > MacroAssembler::Set() did similar things.
> > 
> > But this change seems to be to the effect of changing
> > 
> > if (A) {
> >   B;
> > } else if (C) {
> >   D;
> > }
> > 
> > into
> > 
> > if (C) {
> >   D;
> > } else if (A) {
> >   B;
> > }
> > 
> > I don't see any difference besides changing the order.
> 
> I think that cond A & C overlaps, e.g. is_uint32(0x1) and is_int32(0x1) are
both
> true.

Ah. Good point. Thanks for the explaination. LGTM.

[Yang, 2013-02-06 11:55:16]

Committed manually as r13605 (presubmit successful).