emitIAS for movsx and movzx.

emitIAS for movsx and movzx.

Force dest to be the full 32-bit reg instead of sometimes being
a 16-bit reg. This is to save on a operand size prefix (and
avoid passing the DestTy down to the dispatchers).

BUG=none
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=39d4aca3794dde283c83907322c2c9b632ecaf7d

[jvoung (off chromium), 2014-10-15 19:09:18]

jvoung@chromium.org changed reviewers:
+ stichnot@chromium.org

[Jim Stichnoth, 2014-10-15 20:08:45]

Otherwise LGTM.

https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/test_i1.ll (right):

https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/test_i1.ll:82: ; CHECK: movzx [[REG:.*]], {{[a-d]l|byte
ptr}}
Should this be something like [[REG:e.*]] to indicate the 32-bit dest register?

Also, it puzzled me that the following "and" instruction uses the 32-bit
register instead of 16-bit, but I see that lowering code explicitly uses
IceType_i32.  Something like the diff below seems to work; maybe at least mark
it with a TODO?

diff --git a/src/IceTargetLoweringX8632.cpp b/src/IceTargetLoweringX8632.cpp
index dfb1fda..36ddebe 100644
--- a/src/IceTargetLoweringX8632.cpp
+++ b/src/IceTargetLoweringX8632.cpp
@@ -2018,8 +2018,11 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
     } else if (Src0RM->getType() == IceType_i1) {
       // t = Src0RM; t &= 1; Dest = t
       Constant *One = Ctx->getConstantInt32(IceType_i32, 1);
-      Variable *T = makeReg(IceType_i32);
-      _movzx(T, Src0RM);
+      Variable *T = makeReg(Dest->getType());
+      if (Dest->getType() == IceType_i8)
+        _mov(T, Src0RM);
+      else
+        _movzx(T, Src0RM);
       _and(T, One);
       _mov(Dest, T);
     } else {

[jvoung (off chromium), 2014-10-15 21:07:29]

https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/test_i1.ll (right):

https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/test_i1.ll:82: ; CHECK: movzx [[REG:.*]], {{[a-d]l|byte
ptr}}
On 2014/10/15 20:08:45, stichnot wrote:
> Should this be something like [[REG:e.*]] to indicate the 32-bit dest
register?
> 
> Also, it puzzled me that the following "and" instruction uses the 32-bit
> register instead of 16-bit, but I see that lowering code explicitly uses
> IceType_i32.  Something like the diff below seems to work; maybe at least mark
> it with a TODO?
> 
> diff --git a/src/IceTargetLoweringX8632.cpp b/src/IceTargetLoweringX8632.cpp
> index dfb1fda..36ddebe 100644
> --- a/src/IceTargetLoweringX8632.cpp
> +++ b/src/IceTargetLoweringX8632.cpp
> @@ -2018,8 +2018,11 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
>      } else if (Src0RM->getType() == IceType_i1) {
>        // t = Src0RM; t &= 1; Dest = t
>        Constant *One = Ctx->getConstantInt32(IceType_i32, 1);
> -      Variable *T = makeReg(IceType_i32);
> -      _movzx(T, Src0RM);
> +      Variable *T = makeReg(Dest->getType());
> +      if (Dest->getType() == IceType_i8)
> +        _mov(T, Src0RM);
> +      else
> +        _movzx(T, Src0RM);
>        _and(T, One);
>        _mov(Dest, T);
>      } else {

Hmm, yes it's a bit puzzling.

Thinking about it more, the 16-bit version will end up being longer than the
32-bit version because of the 66 prefix. Since the immediate is just "1" they
can both take advantage of the shorter imm8 form.

However, the:

> +      if (Dest->getType() == IceType_i8)
> +        _mov(T, Src0RM);

part does help some, because it can notice that the mov is redundant when the
same register is chosen, and eliminate it, while the movzx is not eliminated.
The 8-bit one is also one byte shorter, if the register happens to be "al". On
the other hand, the 32-bit version gives you more choices for registers?

So, would it make sense to use the Dest type for 8-bit only, and then leave a
comment about 16-bit vs 32-bit?

[Jim Stichnoth, 2014-10-15 21:59:34]

On 2014/10/15 21:07:29, jvoung wrote:
>
https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
> File tests_lit/llvm2ice_tests/test_i1.ll (right):
> 
>
https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
> tests_lit/llvm2ice_tests/test_i1.ll:82: ; CHECK: movzx [[REG:.*]],
{{[a-d]l|byte
> ptr}}
> On 2014/10/15 20:08:45, stichnot wrote:
> > Should this be something like [[REG:e.*]] to indicate the 32-bit dest
> register?
> > 
> > Also, it puzzled me that the following "and" instruction uses the 32-bit
> > register instead of 16-bit, but I see that lowering code explicitly uses
> > IceType_i32.  Something like the diff below seems to work; maybe at least
mark
> > it with a TODO?
> > 
> > diff --git a/src/IceTargetLoweringX8632.cpp b/src/IceTargetLoweringX8632.cpp
> > index dfb1fda..36ddebe 100644
> > --- a/src/IceTargetLoweringX8632.cpp
> > +++ b/src/IceTargetLoweringX8632.cpp
> > @@ -2018,8 +2018,11 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
> >      } else if (Src0RM->getType() == IceType_i1) {
> >        // t = Src0RM; t &= 1; Dest = t
> >        Constant *One = Ctx->getConstantInt32(IceType_i32, 1);
> > -      Variable *T = makeReg(IceType_i32);
> > -      _movzx(T, Src0RM);
> > +      Variable *T = makeReg(Dest->getType());
> > +      if (Dest->getType() == IceType_i8)
> > +        _mov(T, Src0RM);
> > +      else
> > +        _movzx(T, Src0RM);
> >        _and(T, One);
> >        _mov(Dest, T);
> >      } else {
> 
> Hmm, yes it's a bit puzzling.
> 
> Thinking about it more, the 16-bit version will end up being longer than the
> 32-bit version because of the 66 prefix. Since the immediate is just "1" they
> can both take advantage of the shorter imm8 form.
> 
> However, the:
> 
> > +      if (Dest->getType() == IceType_i8)
> > +        _mov(T, Src0RM);
> 
> part does help some, because it can notice that the mov is redundant when the
> same register is chosen, and eliminate it, while the movzx is not eliminated.
> The 8-bit one is also one byte shorter, if the register happens to be "al". On
> the other hand, the 32-bit version gives you more choices for registers?
> 
> So, would it make sense to use the Dest type for 8-bit only, and then leave a
> comment about 16-bit vs 32-bit?

I think that internal type consistency is more important than code size, and
type consistency is especially tricky around the type conversions.  This is
probably why there is a movzx instruction in the first place, given that the
"and" instruction makes it redundant.  Anyway, maybe just best to leave a TODO
and move on. :)

[Jim Stichnoth, 2014-10-15 22:01:33]

On 2014/10/15 21:59:34, stichnot wrote:
> On 2014/10/15 21:07:29, jvoung wrote:
> >
>
https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
> > File tests_lit/llvm2ice_tests/test_i1.ll (right):
> > 
> >
>
https://codereview.chromium.org/647223004/diff/40001/tests_lit/llvm2ice_tests...
> > tests_lit/llvm2ice_tests/test_i1.ll:82: ; CHECK: movzx [[REG:.*]],
> {{[a-d]l|byte
> > ptr}}
> > On 2014/10/15 20:08:45, stichnot wrote:
> > > Should this be something like [[REG:e.*]] to indicate the 32-bit dest
> > register?
> > > 
> > > Also, it puzzled me that the following "and" instruction uses the 32-bit
> > > register instead of 16-bit, but I see that lowering code explicitly uses
> > > IceType_i32.  Something like the diff below seems to work; maybe at least
> mark
> > > it with a TODO?
> > > 
> > > diff --git a/src/IceTargetLoweringX8632.cpp
b/src/IceTargetLoweringX8632.cpp
> > > index dfb1fda..36ddebe 100644
> > > --- a/src/IceTargetLoweringX8632.cpp
> > > +++ b/src/IceTargetLoweringX8632.cpp
> > > @@ -2018,8 +2018,11 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
> > >      } else if (Src0RM->getType() == IceType_i1) {
> > >        // t = Src0RM; t &= 1; Dest = t
> > >        Constant *One = Ctx->getConstantInt32(IceType_i32, 1);
> > > -      Variable *T = makeReg(IceType_i32);
> > > -      _movzx(T, Src0RM);
> > > +      Variable *T = makeReg(Dest->getType());
> > > +      if (Dest->getType() == IceType_i8)
> > > +        _mov(T, Src0RM);
> > > +      else
> > > +        _movzx(T, Src0RM);
> > >        _and(T, One);
> > >        _mov(Dest, T);
> > >      } else {
> > 
> > Hmm, yes it's a bit puzzling.
> > 
> > Thinking about it more, the 16-bit version will end up being longer than the
> > 32-bit version because of the 66 prefix. Since the immediate is just "1"
they
> > can both take advantage of the shorter imm8 form.
> > 
> > However, the:
> > 
> > > +      if (Dest->getType() == IceType_i8)
> > > +        _mov(T, Src0RM);
> > 
> > part does help some, because it can notice that the mov is redundant when
the
> > same register is chosen, and eliminate it, while the movzx is not
eliminated.
> > The 8-bit one is also one byte shorter, if the register happens to be "al".
On
> > the other hand, the 32-bit version gives you more choices for registers?
> > 
> > So, would it make sense to use the Dest type for 8-bit only, and then leave
a
> > comment about 16-bit vs 32-bit?
> 
> I think that internal type consistency is more important than code size, and
> type consistency is especially tricky around the type conversions.  This is
> probably why there is a movzx instruction in the first place, given that the
> "and" instruction makes it redundant.  Anyway, maybe just best to leave a TODO
> and move on. :)

Whoops, didn't notice you had uploaded a new patch.  Still LGTM.

[jvoung (off chromium), 2014-10-15 22:16:59]

Committed patchset #5 (id:80001) manually as
39d4aca3794dde283c83907322c2c9b632ecaf7d (presubmit successful).