Add VADD instruction to the ARM integrated assembler.

Add VADD instruction to the ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=stichnot@chromium.org

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

[Karl, 2015-12-18 23:42:29]

Description was changed from

==========
Add VADD instruction to the ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add VADD instruction to the ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2015-12-18 23:42:29]

kschimpf@google.com changed reviewers:
+ jpp@chromium.org, sehr@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2015-12-19 15:31:25]

https://codereview.chromium.org/1540653003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1540653003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:143: bool isGPRRegisterDefined(IValueT R) { return R
<= RegARM32::getNumGPRRegs(); }
This "<=" looked like a bug to me, until I looked at the implementation of
getNumGPRRegs() and saw that it's not actually the *number* of GPRs, but rather
the encoding of the highest-numbered GPR.

I think it would be more clear and more natural if you change "<=" to "<", and
remove the "-1" from getNumGPRRegs()'s implementation.

https://codereview.chromium.org/1540653003/diff/20001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1540653003/diff/20001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:124: // Note: D registers are listed from largest to
smallest.
We should not be assuming any ordering (including S and Q and GP regs).  In
fact, we shouldn't even assume they are contiguous.

The IceRegistersARM32.def file has a 0/1 column indicating the register's type
(IsInt, IsFP32, etc.), and also the register's encoded value.  Can these be used
instead of relying on the _First and _Last values?

[Karl, 2016-01-05 20:24:32]

https://codereview.chromium.org/1540653003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1540653003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:143: bool isGPRRegisterDefined(IValueT R) { return R
<= RegARM32::getNumGPRRegs(); }
On 2015/12/19 15:31:25, stichnot wrote:
> This "<=" looked like a bug to me, until I looked at the implementation of
> getNumGPRRegs() and saw that it's not actually the *number* of GPRs, but
rather
> the encoding of the highest-numbered GPR.
> 
> I think it would be more clear and more natural if you change "<=" to "<", and
> remove the "-1" from getNumGPRRegs()'s implementation.


The problem here is that R is the assembler encoded value of the register, not
Subzero's encoded value. It also assumes that the register is defined. However,
the checking for undefined Subzero's encoded value should have happened much
earlier.

Changing the code to check for undefined register while still modeled as a
Subzero encoded register, and removing these (late) checking functions.

https://codereview.chromium.org/1540653003/diff/20001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1540653003/diff/20001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:124: // Note: D registers are listed from largest to
smallest.
On 2015/12/19 15:31:25, stichnot wrote:
> We should not be assuming any ordering (including S and Q and GP regs).  In
> fact, we shouldn't even assume they are contiguous.
> 
> The IceRegistersARM32.def file has a 0/1 column indicating the register's type
> (IsInt, IsFP32, etc.), and also the register's encoded value.  Can these be
used
> instead of relying on the _First and _Last values?

If you note the functions above this one, ALL of the encoding functions assume
that each category is contiguous. So, I followed that code. However, I agree
that we have the corresponding information in the X macro to implement
getEncodedXReg methods.

For set inclusion, I'm not sure that we can use the type. For example, general
purpose registers r9, ip, sp, lr, and pc are NOT integer. Talked to jpp to fix
this (now done). Modifying code.

[Jim Stichnoth, 2016-01-06 18:39:14]

https://codereview.chromium.org/1540653003/diff/60001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1540653003/diff/60001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:296: (void)WantedRegSet;
remove this

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:149: static inline bool isRegisterDefined(int32_t
RegNum) {
This function bothers me (perhaps more than it should).

In the rest of the code base (specifically thinking of lowering, register
allocation, and x86 emission), the int32_t RegNum is gotten from
Variable::getRegNum(), perhaps guarded by Variable::hasReg() or
Variable::NoRegister.  Then the emitter just uses it without any test like this
isRegisterDefined().  That makes me think that maybe there is something special
and deeper about register numbers in ARM.

But that's not true, I think.  It looks like the only use of isRegisterDefined()
is to assert that it is true.

To make that more clear, I would change this to something like:

static inline void assertRegisterDefined(int32_t RegNum) {
  assert(RegNum >= 0 && RegNum < Reg_NUM);
}

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:158: static inline SizeT getNumGPRegs() {
Can these getNumRegs() functions be defined as constexpr?

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:188: return Reg_SREG_Last + 1 - Reg_SREG_First;
Have I mentioned that I ***REALLY*** dislike assuming that the register enum
values are in a certain order.

I see two fairly reasonable ways to handle it.  First:

static inline SizeT getNumSRegs() {
  if (Reg_SREG_Last > Reg_SREG_First)
    return Reg_SREG_Last + 1 - Reg_SREG_First;
  return Reg_SREG_First + 1 - Reg_SREG_Last;
}

The other way is a bit more complicated (x-macros) but doesn't assume that First
and Last are at the endpoints of the enum value range.  Instead, just count the
number of "1" entries in the isFP32 column of the register table:

static inline SizeT getNumSRegs() {
  return 0
#define X(val, encode, name, cc_arg, scratch, preserved, stackptr, frameptr,   \
          isGPR, isInt, isI64Pair, isFP32, isFP64, isVec128, alias_init)       \
    + (isFP32)
    REGARM32_TABLE
#undef X
  ;
}

Or for a more syntactically structured version:

static inline SizeT getNumSRegs() {
  SizeT Count = 0;
#define X(val, encode, name, cc_arg, scratch, preserved, stackptr, frameptr,   \
          isGPR, isInt, isI64Pair, isFP32, isFP64, isVec128, alias_init)       \
  Count += (isFP32);
  REGARM32_TABLE
#undef X
  return Count;
}

though I'm not sure if this would work if getNumSRegs() is a constexpr function.

This comment applies to getNumDRegs(), and is also relevant to getNumGPRegs(). 
The x-macro implementation of getNumGPRegs() would need to sum up the isGPR
column, not the isInt column.

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:196: // Note: D registers are listed from largest to
smallest.
Move this down to getNumDRegs(), assuming it's still relevant with respect to
the CL comment above.

[Karl, 2016-01-07 00:00:35]

https://codereview.chromium.org/1540653003/diff/60001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1540653003/diff/60001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:296: (void)WantedRegSet;
On 2016/01/06 18:39:14, stichnot wrote:
> remove this

Done.

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:149: static inline bool isRegisterDefined(int32_t
RegNum) {
On 2016/01/06 18:39:14, stichnot wrote:
> This function bothers me (perhaps more than it should).
> 
> In the rest of the code base (specifically thinking of lowering, register
> allocation, and x86 emission), the int32_t RegNum is gotten from
> Variable::getRegNum(), perhaps guarded by Variable::hasReg() or
> Variable::NoRegister.  Then the emitter just uses it without any test like
this
> isRegisterDefined().  That makes me think that maybe there is something
special
> and deeper about register numbers in ARM.
> 
> But that's not true, I think.  It looks like the only use of
isRegisterDefined()
> is to assert that it is true.
> 
> To make that more clear, I would change this to something like:
> 
> static inline void assertRegisterDefined(int32_t RegNum) {
>   assert(RegNum >= 0 && RegNum < Reg_NUM);
> }

I agree that the check is an assertion, since enums GPRRegister, SRegister,
DRegister, and QRegister define an "Encoded_Not_... = -1" value. Fixing to use
assert in name.

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:158: static inline SizeT getNumGPRegs() {
On 2016/01/06 18:39:14, stichnot wrote:
> Can these getNumRegs() functions be defined as constexpr?

Done.

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:188: return Reg_SREG_Last + 1 - Reg_SREG_First;
On 2016/01/06 18:39:14, stichnot wrote:
> Have I mentioned that I ***REALLY*** dislike assuming that the register enum
> values are in a certain order.
> 
> I see two fairly reasonable ways to handle it.  First:
> 
> static inline SizeT getNumSRegs() {
>   if (Reg_SREG_Last > Reg_SREG_First)
>     return Reg_SREG_Last + 1 - Reg_SREG_First;
>   return Reg_SREG_First + 1 - Reg_SREG_Last;
> }
> 
> The other way is a bit more complicated (x-macros) but doesn't assume that
First
> and Last are at the endpoints of the enum value range.  Instead, just count
the
> number of "1" entries in the isFP32 column of the register table:
> 
> static inline SizeT getNumSRegs() {
>   return 0
> #define X(val, encode, name, cc_arg, scratch, preserved, stackptr, frameptr,  
\
>           isGPR, isInt, isI64Pair, isFP32, isFP64, isVec128, alias_init)      
\
>     + (isFP32)
>     REGARM32_TABLE
> #undef X
>   ;
> }
> 
> Or for a more syntactically structured version:
> 
> static inline SizeT getNumSRegs() {
>   SizeT Count = 0;
> #define X(val, encode, name, cc_arg, scratch, preserved, stackptr, frameptr,  
\
>           isGPR, isInt, isI64Pair, isFP32, isFP64, isVec128, alias_init)      
\
>   Count += (isFP32);
>   REGARM32_TABLE
> #undef X
>   return Count;
> }
> 
> though I'm not sure if this would work if getNumSRegs() is a constexpr
function.
> 
> This comment applies to getNumDRegs(), and is also relevant to getNumGPRegs().

> The x-macro implementation of getNumGPRegs() would need to sum up the isGPR
> column, not the isInt column.

Changed form of count functions to use REGARM32_TABLE

https://codereview.chromium.org/1540653003/diff/60001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:196: // Note: D registers are listed from largest to
smallest.
On 2016/01/06 18:39:14, stichnot wrote:
> Move this down to getNumDRegs(), assuming it's still relevant with respect to
> the CL comment above.

Removed. No longer applicable.

[Jim Stichnoth, 2016-01-07 01:11:17]

lgtm

[Karl, 2016-01-07 15:31:23]

Description was changed from

==========
Add VADD instruction to the ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add VADD instruction to the ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=stichnot@chromium.org

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

[Karl, 2016-01-07 15:31:26]

Committed patchset #5 (id:80001) manually as
4acf11ac13716f5998b56cb65c1c72341276cc5f (presubmit successful).