Subzero. Changes the declaration for ARM32 registers.

Subzero. Changes the declaration for ARM32 registers.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

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

[John, 2015-09-04 18:36:36]

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

[John, 2015-09-04 18:37:05]

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

[John, 2015-09-04 18:37:05]

Sorry Jan, my bad. feel free to ignore this.

[Jim Stichnoth, 2015-09-04 20:24:16]

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcode78
src/IceInstARM32.def:78: //            'ALIASES(Reg_s{regnum:<2},
Reg_s{regnum_s:<2}, ' +
Should the second Reg_s be Reg_d?  (and same for the resulting output lines
below)

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcod...
src/IceInstARM32.def:255: // are also scratch (if supported by the D32 feature).
Trailing space?

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcod...
src/IceInstARM32.def:281: //
Trailing space?

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcod...
src/IceInstARM32.def:386: X(EQ, 0 , NE, "eq")   /* equal */                     
                       \
80-col

https://codereview.chromium.org/1310863007/diff/1/src/IceTargetLoweringARM32.cpp
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1310863007/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.cpp:474: *Reg = RegARM32::Reg_q0 - (NumFPRegUnits /
4);
How about something like:

  // Handle reg numbering in forward or reverse order.
  constexpr int AscOrDesc = RegARM32::Reg_q1 - RegARM32::Reg_q0;
  static_assert(AscOrDesc == 1 || AscOrDesc == -1, "oops");
  *Reg = RegARM32::Reg_q0 + AscOrDesc * (NumFPRegUnits / 4);

[John, 2015-09-04 21:23:25]

Please take another look. In particular, if think the IncDec trick is better
than my (new) solution, I am happy to change.

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcode78
src/IceInstARM32.def:78: //            'ALIASES(Reg_s{regnum:<2},
Reg_s{regnum_s:<2}, ' +
On 2015/09/04 20:24:16, stichnot wrote:
> Should the second Reg_s be Reg_d?  (and same for the resulting output lines
> below)

maybe... I'll add an assert to the initialization code.

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcod...
src/IceInstARM32.def:255: // are also scratch (if supported by the D32 feature).
On 2015/09/04 20:24:16, stichnot wrote:
> Trailing space?

Done.

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcod...
src/IceInstARM32.def:281: //
On 2015/09/04 20:24:16, stichnot wrote:
> Trailing space?

Done.

https://codereview.chromium.org/1310863007/diff/1/src/IceInstARM32.def#newcod...
src/IceInstARM32.def:386: X(EQ, 0 , NE, "eq")   /* equal */                     
                       \
On 2015/09/04 20:24:16, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1310863007/diff/1/src/IceTargetLoweringARM32.cpp
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1310863007/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.cpp:474: *Reg = RegARM32::Reg_q0 - (NumFPRegUnits /
4);
On 2015/09/04 20:24:16, stichnot wrote:
> How about something like:
> 
>   // Handle reg numbering in forward or reverse order.
>   constexpr int AscOrDesc = RegARM32::Reg_q1 - RegARM32::Reg_q0;
>   static_assert(AscOrDesc == 1 || AscOrDesc == -1, "oops");
>   *Reg = RegARM32::Reg_q0 + AscOrDesc * (NumFPRegUnits / 4);

Not a fan -- but I also hate the current approach.

I added a comment stating why we do minus instead of plus, and a static_assert,
just in case.

[Jim Stichnoth, 2015-09-04 22:08:36]

lgtm

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:153: //          isInt, isFP32,disFP64, isVec128,
aliases_init)
isFP64

https://codereview.chromium.org/1310863007/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1310863007/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:193: "Duplicate alias for " #val);               
                       \
cool

https://codereview.chromium.org/1310863007/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:499: *Reg = RegARM32::Reg_s0 + NumFPRegUnits;
I think you should add the same kind of static_assert here as well.

[jvoung (off chromium), 2015-09-08 16:01:16]

well, I had some free time -- looks good =)

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:49: isInt, isFP32, isFP64, isVec128, aliases_init */       
                  \
nit: a bunch of other places use "alias_init" instead of "aliases_init"

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:78: //            'ALIASES(Reg_s{regnum_s:<2},
Reg_d{regnum:<2}, ' +
regnum_s -> regnum
regnum -> regnum_d
ALIASES -> ALIASES3
?

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:160: // allocating "high" D registers, which do not alias
any S registers.
nice

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:169: //            'ALIASES(Reg_d{regnum:<2},
Reg_q{regnum_q:<2})    \\').format(
ALIASES2

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:177: //            'ALIASES(Reg_s{regnum_s0:<2},
Reg_s{regnum_s1:<2}, ' +
ALIASES4

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:264: //            '{scratch}, {preserved}, 0, 0, 0, 0, 0,
1, ALIASES(' +
ALIASES3

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:273: //            '{scratch}, {preserved}, 0, 0, 0, 0, 0,
1, ALIASES(' +
ALIASES7

[John, 2015-09-08 16:01:38]

this is landing now.

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1310863007/diff/20001/src/IceInstARM32.def#ne...
src/IceInstARM32.def:153: //          isInt, isFP32,disFP64, isVec128,
aliases_init)
On 2015/09/04 22:08:36, stichnot wrote:
> isFP64

Done.

https://codereview.chromium.org/1310863007/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1310863007/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:193: "Duplicate alias for " #val);               
                       \
On 2015/09/04 22:08:36, stichnot wrote:
> cool

Acknowledged.

https://codereview.chromium.org/1310863007/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:499: *Reg = RegARM32::Reg_s0 + NumFPRegUnits;
On 2015/09/04 22:08:36, stichnot wrote:
> I think you should add the same kind of static_assert here as well.

Done.

[John, 2015-09-08 16:03:26]

Committed patchset #3 (id:40001) manually as
5300bfe5c1b6dd711f77284691c920fc6c830873 (presubmit successful).