Subzero. ARM32. Introduces explicit register parameter attribute.

Subzero. ARM32. Introduces explicit register parameter attribute.

The ARM32 backend used to rely on a specific register declaration order
for calling convention register assignment. This CL introduces a new
field in the ARM32 register tables for explicitly setting which
register holds which parameter.

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=2187c84aaf770db0e70024d8781c414e270625a0

[John, 2015-12-09 22:52:49]

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

[Jim Stichnoth, 2015-12-10 19:04:06]

https://codereview.chromium.org/1508423003/diff/100001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1508423003/diff/100001/src/IceInstARM32.def#n...
src/IceInstARM32.def:38: X(Reg_r0,  0,  "r0",  1, 1, 0, 0, 0, 1, 0, 0, 0, 0,    
                     \
I realize that generation of these tables is now entirely scripted, so take the
following comments as completely optional.

1. If, during development or debugging, I want to look at this table, I really
want to look at this instantiation of it rather than the claimed-source-of-truth
.py file.  As such, I would very much like to try to maintain 80-col.

2. The original table packs somewhat-related columns close together by not using
a space character after a comma.  This makes it easier to visually locate a
column from some row deep in the table.  (It also helps preserve 80-col.)

3. The original table deliberately omits listing trivial self-aliases.  This
makes the table easier to read (IMO) by minimizing basically redundant
information.  (It also helps preserve 80-col.)

4. Many columns are "sparse" in that they contain mostly zeroes.  It might be
more readable (or maybe not) if "0" in such a column is replaced with a single
space character.  Then e.g. "blah = isInt;" in an x-macro is replaced with "blah
= isInt + 0;".

5. It would be really cool if there were a good way to remove the redundant
"RegARM32" from every X() line.  I originally tried but this is the best I could
do.

Regarding #1, maybe the Makefile could invoke the .py file to auto-generate
another .def file to #include here, and completely ignore the legacy 80-col
issue.

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:167: +(((cc_arg > 0)) ? 1 : 0)
You could replace this expression with the simpler:
  +(cc_arg > 0)
In the big picture, I think that makes it somewhat easier to understand.

If not, at least change
  ((cc_arg > 0))
to
  (cc_arg > 0)
Or maybe you meant
  ((cc_arg) > 0)
because cc_arg is a macro parameter?

Finally, my OCD balks at "+((..." rather than "+ ((...".  I wonder if
clang-format would let the latter through.

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:935: // Source->back() is marked as unavailable,
and it is thus popped from the
maybe "thus implicitly popped"?

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:941: // GPR are not package when passing
parameters. Thus, a funtion foo(i32, i64,
"are not packed" (I think)

function

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1023: default: { RegisterArg->setRegNum(RegNum);
} break;
This seems very unlike clang-format...

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1053: /// type, and false otherwise. If if returns
true, Reg is set to the
If it

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1063: void
discardUnavailableGPRsAndTheirAliases(std::vector<SizeT> *Regs);
Can you use CfgVector instead of std::vector?

[Jim Stichnoth, 2015-12-11 06:17:13]

https://codereview.chromium.org/1508423003/diff/100001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1508423003/diff/100001/src/IceInstARM32.def#n...
src/IceInstARM32.def:38: X(Reg_r0,  0,  "r0",  1, 1, 0, 0, 0, 1, 0, 0, 0, 0,    
                     \
On 2015/12/10 19:04:05, stichnot wrote:
> 4. Many columns are "sparse" in that they contain mostly zeroes.  It might be
> more readable (or maybe not) if "0" in such a column is replaced with a single
> space character.  Then e.g. "blah = isInt;" in an x-macro is replaced with
"blah
> = isInt + 0;".

Actually, I took another look at the .py file, and I see that the sparsity is
already represented there.  Cool!

> Regarding #1, maybe the Makefile could invoke the .py file to auto-generate
> another .def file to #include here, and completely ignore the legacy 80-col
> issue.

If you do this (and after more consideration I think you should), I would make
the auto-generated .def file be officially checked into the repo (as opposed to
being added to .gitignore).  That way you don't have to figure out how to modify
the Makefile to auto-gen the .def file as needed.

[John, 2015-12-11 15:43:30]

https://codereview.chromium.org/1508423003/diff/100001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1508423003/diff/100001/src/IceInstARM32.def#n...
src/IceInstARM32.def:38: X(Reg_r0,  0,  "r0",  1, 1, 0, 0, 0, 1, 0, 0, 0, 0,    
                     \
I added the other .def file. In the future I will #include it where it is needed
-- .def #include another .def is confusing.

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:167: +(((cc_arg > 0)) ? 1 : 0)
On 2015/12/10 19:04:05, stichnot wrote:
> You could replace this expression with the simpler:
>   +(cc_arg > 0)
> In the big picture, I think that makes it somewhat easier to understand.
> 
> If not, at least change
>   ((cc_arg > 0))
> to
>   (cc_arg > 0)
> Or maybe you meant
>   ((cc_arg) > 0)
> because cc_arg is a macro parameter?
I really dislike adding booleans, hence the ((cc_arg) > 0) ? 1 : 0. (yup, the
inner rparen was misplaced.)

> 
> Finally, my OCD balks at "+((..." rather than "+ ((...".  I wonder if
> clang-format would let the latter through.
It doesn't. :-/

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:935: // Source->back() is marked as unavailable,
and it is thus popped from the
On 2015/12/10 19:04:06, stichnot wrote:
> maybe "thus implicitly popped"?

it is not implicit, it is explicitly popped by a pop_back() call. Anyway, you're
the native English speaker here, so I'll trust your judgement on this one. :)

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:941: // GPR are not package when passing
parameters. Thus, a funtion foo(i32, i64,
On 2015/12/10 19:04:05, stichnot wrote:
> "are not packed" (I think)
> 
> function

Done.

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1023: default: { RegisterArg->setRegNum(RegNum);
} break;
On 2015/12/10 19:04:05, stichnot wrote:
> This seems very unlike clang-format...

... but  it is its fault: I just tried it.

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1053: /// type, and false otherwise. If if returns
true, Reg is set to the
On 2015/12/10 19:04:06, stichnot wrote:
> If it

Done.

https://codereview.chromium.org/1508423003/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1063: void
discardUnavailableGPRsAndTheirAliases(std::vector<SizeT> *Regs);
On 2015/12/10 19:04:06, stichnot wrote:
> Can you use CfgVector instead of std::vector?

Done.

[Jim Stichnoth, 2015-12-11 18:47:41]

https://codereview.chromium.org/1508423003/diff/120001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1508423003/diff/120001/Makefile.standalone#ne...
Makefile.standalone:329: echo "IceRegistersARM32.def is out-of-date. Please
regenerate it with " \
80-col, thanks tab characters!

Could this makefile just regenerate the .def file if it's out of date?

https://codereview.chromium.org/1508423003/diff/120001/pydir/gen_arm32_reg_ta...
File pydir/gen_arm32_reg_tables.py (right):

https://codereview.chromium.org/1508423003/diff/120001/pydir/gen_arm32_reg_ta...
pydir/gen_arm32_reg_tables.py:212: "// This file was auto generated by the
gen_arm32_reg_tables.py script. "
Could you make this the very very first line of the .def file?

https://codereview.chromium.org/1508423003/diff/120001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1508423003/diff/120001/src/IceInstARM32.def#n...
src/IceInstARM32.def:36: // The registers tables define in IceRegistersARM32 use
the following X-macro:
I would say "register tables defined"

Also, for better or worse, the code base lowercases "x-macro" unless it starts a
sentence (fragment).

https://codereview.chromium.org/1508423003/diff/120001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1508423003/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:176: +(((cc_arg > 0)) ? 1 : 0)
Also change the parentheses here and below.

[Jim Stichnoth, 2015-12-11 20:15:25]

https://codereview.chromium.org/1508423003/diff/120001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1508423003/diff/120001/Makefile.standalone#ne...
Makefile.standalone:329: echo "IceRegistersARM32.def is out-of-date. Please
regenerate it with " \
On 2015/12/11 18:47:41, stichnot wrote:
> Could this makefile just regenerate the .def file if it's out of date?

Answering a question posed offline...

We can assume that Makefile.standalone is run in an environment that has python
available.  After all, "check-lit" uses lit which uses python; "check-xtest"
uses crosstest python scripts; "check-spec" uses the szbuild scripts.

The "official" builder builds Subzero as part of the greater LLVM build, and
uses Makefile rather than Makefile.standalone.  Under this development model,
any changes to the .py file might result in changes to the .def file after the
Makefile.standalone reruns the .py script, and those changes would become part
of the CL.  The script might spuriously fire due to timestamp issues, but as
long as it produces the same output, all should be fine.

[John, 2015-12-11 20:23:32]

https://codereview.chromium.org/1508423003/diff/120001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1508423003/diff/120001/Makefile.standalone#ne...
Makefile.standalone:329: echo "IceRegistersARM32.def is out-of-date. Please
regenerate it with " \
On 2015/12/11 18:47:41, stichnot wrote:
> 80-col, thanks tab characters!
> 
> Could this makefile just regenerate the .def file if it's out of date?

Done.

https://codereview.chromium.org/1508423003/diff/120001/pydir/gen_arm32_reg_ta...
File pydir/gen_arm32_reg_tables.py (right):

https://codereview.chromium.org/1508423003/diff/120001/pydir/gen_arm32_reg_ta...
pydir/gen_arm32_reg_tables.py:212: "// This file was auto generated by the
gen_arm32_reg_tables.py script. "
On 2015/12/11 18:47:41, stichnot wrote:
> Could you make this the very very first line of the .def file?

I wanted to avoid "leaking" this comment when the file had already been
included.

https://codereview.chromium.org/1508423003/diff/120001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1508423003/diff/120001/src/IceInstARM32.def#n...
src/IceInstARM32.def:36: // The registers tables define in IceRegistersARM32 use
the following X-macro:
On 2015/12/11 18:47:41, stichnot wrote:
> I would say "register tables defined"
> 
> Also, for better or worse, the code base lowercases "x-macro" unless it starts
a
> sentence (fragment).

Done.

https://codereview.chromium.org/1508423003/diff/120001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1508423003/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:176: +(((cc_arg > 0)) ? 1 : 0)
On 2015/12/11 18:47:41, stichnot wrote:
> Also change the parentheses here and below.

Done.

[Jim Stichnoth, 2015-12-11 21:22:10]

lgtm

https://codereview.chromium.org/1508423003/diff/140001/pydir/gen_arm32_reg_ta...
File pydir/gen_arm32_reg_tables.py (right):

https://codereview.chromium.org/1508423003/diff/140001/pydir/gen_arm32_reg_ta...
pydir/gen_arm32_reg_tables.py:209: print ("// This file was auto generated by
the gen_arm32_reg_tables.py script. "
It would be extra cool if the "gen_arm32_reg_tables.py" string could come from
argv[0] or whatever, to make it more future-proof.

[John, 2015-12-16 15:48:28]

Description was changed from

==========
Subzero. ARM32. Introduces explicit register parameter attribute.

The ARM32 backend used to rely on a specific register declaration order
for calling convention register assignment. This CL introduces a new
field in the ARM32 register tables for explicitly setting which
register holds which parameter.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Introduces explicit register parameter attribute.

The ARM32 backend used to rely on a specific register declaration order
for calling convention register assignment. This CL introduces a new
field in the ARM32 register tables for explicitly setting which
register holds which parameter.

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...
==========

[John, 2015-12-16 15:48:29]

Committed patchset #10 (id:180001) manually as
2187c84aaf770db0e70024d8781c414e270625a0 (presubmit successful).

[John, 2015-12-16 15:51:14]

https://codereview.chromium.org/1508423003/diff/140001/pydir/gen_arm32_reg_ta...
File pydir/gen_arm32_reg_tables.py (right):

https://codereview.chromium.org/1508423003/diff/140001/pydir/gen_arm32_reg_ta...
pydir/gen_arm32_reg_tables.py:209: print ("// This file was auto generated by
the gen_arm32_reg_tables.py script. "
On 2015/12/11 21:22:10, stichnot wrote:
> It would be extra cool if the "gen_arm32_reg_tables.py" string could come from
> argv[0] or whatever, to make it more future-proof.

Done.