Subzero: Refactor x86 register representation to actively use aliases.

Subzero: Refactor x86 register representation to actively use aliases.

Sets up additional register attributes, plus the notion of register classes, to enable robust usage of the high 8-bit GPRs (ah/bh/ch/dh), for both x86-32 and x86-64.  (Note that the x86-64 changes are currently untested.)

We add a Register Class field to the Variable class.  The default register class is a value corresponding to the variable's type, but the target can extend the set of register class values, and the target lowering can assign different register classes as needed.  The register allocator uses the register class instead of the type to determine the set of registers to draw from.

For x86-64, the high 8-bit registers are not included in the general register allocation pool, but there are explicit references to ah for lowering the div/rem instructions.

The target lowering is modified as needed to make sure types are appropriate and register use in instructions is legalized.

Some other fixes and cleanups are included in this CL:

* Makefile.standalone changes.  Source files are reordered so that the more expensive compiles are done earlier, speeding up parallel builds by decreasing fragmentation.  A dependency error is fixed for check-spec.

* A bug is fixed in advanced phi lowering.  When a temporary is introduced to break a cycle, we were neglecting to updated the predecessor count for one of the operands, leading to an assertion failure.  (Applying that fix to master resulted in no changes to spec2k code generation.)  A consistency check is added to help find future problems like this.  Also, refactored iteration over the Phi descriptor array to use range-based for loops and avoid directly indexing the array.

* Removed most of the "IceType_" prefixes in x-macro tables for brevity.

* Fix a correctness TODO in the register allocator.  This had no effect on spec2k code generation in master or in this CL, so we were probably just lucky.

* Made some much-needed s/Dest->getType()/Ty/ changes for brevity, in the target lowering sections that needed other changes.

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

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

[Jim Stichnoth, 2015-11-08 16:58:01]

Description was changed from

==========
Subzero: Refactor x86 register representation to actively use aliases.

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

to

==========
Subzero: Refactor x86 register representation to actively use aliases.

Sets up additional register attributes, plus the notion of register classes, to
enable robust usage of the high 8-bit GPRs (ah/bh/ch/dh), for both x86-32 and
x86-64.  (Note that the x86-64 changes are currently untested.)

We add a Register Class field to the Variable class.  The default register class
is a value corresponding to the variable's type, but the target can extend the
set of register class values, and the target lowering can assign different
register classes as needed.  The register allocator uses the register class
instead of the type to determine the set of registers to draw from.

For x86-64, the high 8-bit registers are not included in the general register
allocation pool, but there are explicit references to ah for lowering the
div/rem instructions.

The target lowering is modified as needed to make sure types are appropriate and
register use in instructions is legalized.

Some other fixes and cleanups are included in this CL:

* Makefile.standalone changes.  Source files are reordered so that the more
expensive compiles are done earlier, speeding up parallel builds by decreasing
fragmentation.  "check-spec" is added to the "check" target because check-spec
is fast enough.  A dependency error is fixed for check-spec.

* A bug is fixed in advanced phi lowering.  When a temporary is introduced to
break a cycle, we were neglecting to updated the predecessor count for one of
the operands, leading to an assertion failure.  (Applying that fix to master
resulted in no changes to spec2k code generation.)  A consistency check is added
to help find future problems like this.  Also, refactored iteration over the Phi
descriptor array to use range-based for loops and avoid directly indexing the
array.

* Removed most of the "IceType_" prefixes in x-macro tables for brevity.

* Fix a correctness TODO in the register allocator.  This had no effect on
spec2k code generation in master or in this CL, so we were probably just lucky.

* Made some much-needed s/Dest->getType()/Ty/ changes for brevity, in the target
lowering sections that needed other changes.

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

[Jim Stichnoth, 2015-11-08 17:44:52]

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

[Jim Stichnoth, 2015-11-08 17:45:59]

Looks like a 12% improvement for bzip2, maybe 3% for gzip, and random
fluctuations elsewhere.  About 1% geomean improvement.

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:521: updatePreds(Desc, Target,
llvm::cast<Variable>(OtherSrc));
This fixes the advanced phi lowering bug mentioned in the CL description.

[John, 2015-11-09 16:23:30]

lgtm

Mostly nits. I would still encourage splitting this CL in two before landing it.

https://codereview.chromium.org/1427973003/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1427973003/diff/20001/Makefile.standalone#new...
Makefile.standalone:428: check: check-lit check-unit check-xtest check-spec
please, don't: check-spec takes a long time in ARM, and it would be nice if I
could keep

make check

for everything else but spec. :)

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:308: PhiDesc(const PhiDesc &) = default;
This struct is somewhat heavy for blindly allowing it to be copied. Make it
movable instead?

PhiDesc(PhiDesc&&) = default

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:412: Desc.emplace_back(PhiDesc(Phi, Dest));
You should use

Desc.emplace(Phi, Dest);

instead. Otherwise, you're invoking two constructors (the explicit ctor you are
invoking, and a copy constructor.)

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:521: updatePreds(Desc, Target,
llvm::cast<Variable>(OtherSrc));
On 2015/11/08 17:45:58, stichnot wrote:
> This fixes the advanced phi lowering bug mentioned in the CL description.

optional: Would you mind sending this in a separate CL, then? We can review it
here, then you can create a simple CL that will be just LGTMed for merging
simplicity (and optimal programmer's experience. :))

https://codereview.chromium.org/1427973003/diff/20001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/1427973003/diff/20001/src/IceInstX8632.def#ne...
src/IceInstX8632.def:58: X(Reg_ah, 4, "ah", Reg_eax, 1,0,0,0, 1,0,0,0,1, 0,
0,0,0,0,1,                \
so this is what turns these registers on? (no pun intended.)

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

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:52: enum RegClassARM32 : uint8_t { RCARM32_NUM =
RC_Target };
This belongs in the IceRegistersARM32.h file, right?

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:81: getRegistersForClass(const Variable *Var) const
override {
This method should be renamed to, e.g., getRegistersForVariable.

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86RegClass.h (right):

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86RegClass.h:25: RCX86_Is64To8 = RC_Target,
optinal: comment each class?

[Jim Stichnoth, 2015-11-09 18:17:36]

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:308: PhiDesc(const PhiDesc &) = default;
On 2015/11/09 16:23:29, John wrote:
> This struct is somewhat heavy for blindly allowing it to be copied. Make it
> movable instead?
> 
> PhiDesc(PhiDesc&&) = default

Done (in the new CL), and then deleted the default copy constructor.

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:412: Desc.emplace_back(PhiDesc(Phi, Dest));
On 2015/11/09 16:23:29, John wrote:
> You should use
> 
> Desc.emplace(Phi, Dest);
> 
> instead. Otherwise, you're invoking two constructors (the explicit ctor you
are
> invoking, and a copy constructor.)

Unfortunately, llvm::SmallVector doesn't provide an emplace() method.

But it does have a push_back method:
  void push_back(T &&Elt) { ... }
Should I use that?

https://codereview.chromium.org/1427973003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:521: updatePreds(Desc, Target,
llvm::cast<Variable>(OtherSrc));
On 2015/11/09 16:23:29, John wrote:
> On 2015/11/08 17:45:58, stichnot wrote:
> > This fixes the advanced phi lowering bug mentioned in the CL description.
> 
> optional: Would you mind sending this in a separate CL, then? We can review it
> here, then you can create a simple CL that will be just LGTMed for merging
> simplicity (and optimal programmer's experience. :))

Done.

[Jim Stichnoth, 2015-11-09 18:19:14]

Description was changed from

==========
Subzero: Refactor x86 register representation to actively use aliases.

Sets up additional register attributes, plus the notion of register classes, to
enable robust usage of the high 8-bit GPRs (ah/bh/ch/dh), for both x86-32 and
x86-64.  (Note that the x86-64 changes are currently untested.)

We add a Register Class field to the Variable class.  The default register class
is a value corresponding to the variable's type, but the target can extend the
set of register class values, and the target lowering can assign different
register classes as needed.  The register allocator uses the register class
instead of the type to determine the set of registers to draw from.

For x86-64, the high 8-bit registers are not included in the general register
allocation pool, but there are explicit references to ah for lowering the
div/rem instructions.

The target lowering is modified as needed to make sure types are appropriate and
register use in instructions is legalized.

Some other fixes and cleanups are included in this CL:

* Makefile.standalone changes.  Source files are reordered so that the more
expensive compiles are done earlier, speeding up parallel builds by decreasing
fragmentation.  "check-spec" is added to the "check" target because check-spec
is fast enough.  A dependency error is fixed for check-spec.

* A bug is fixed in advanced phi lowering.  When a temporary is introduced to
break a cycle, we were neglecting to updated the predecessor count for one of
the operands, leading to an assertion failure.  (Applying that fix to master
resulted in no changes to spec2k code generation.)  A consistency check is added
to help find future problems like this.  Also, refactored iteration over the Phi
descriptor array to use range-based for loops and avoid directly indexing the
array.

* Removed most of the "IceType_" prefixes in x-macro tables for brevity.

* Fix a correctness TODO in the register allocator.  This had no effect on
spec2k code generation in master or in this CL, so we were probably just lucky.

* Made some much-needed s/Dest->getType()/Ty/ changes for brevity, in the target
lowering sections that needed other changes.

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

to

==========
Subzero: Refactor x86 register representation to actively use aliases.

Sets up additional register attributes, plus the notion of register classes, to
enable robust usage of the high 8-bit GPRs (ah/bh/ch/dh), for both x86-32 and
x86-64.  (Note that the x86-64 changes are currently untested.)

We add a Register Class field to the Variable class.  The default register class
is a value corresponding to the variable's type, but the target can extend the
set of register class values, and the target lowering can assign different
register classes as needed.  The register allocator uses the register class
instead of the type to determine the set of registers to draw from.

For x86-64, the high 8-bit registers are not included in the general register
allocation pool, but there are explicit references to ah for lowering the
div/rem instructions.

The target lowering is modified as needed to make sure types are appropriate and
register use in instructions is legalized.

Some other fixes and cleanups are included in this CL:

* Makefile.standalone changes.  Source files are reordered so that the more
expensive compiles are done earlier, speeding up parallel builds by decreasing
fragmentation.  A dependency error is fixed for check-spec.

* A bug is fixed in advanced phi lowering.  When a temporary is introduced to
break a cycle, we were neglecting to updated the predecessor count for one of
the operands, leading to an assertion failure.  (Applying that fix to master
resulted in no changes to spec2k code generation.)  A consistency check is added
to help find future problems like this.  Also, refactored iteration over the Phi
descriptor array to use range-based for loops and avoid directly indexing the
array.

* Removed most of the "IceType_" prefixes in x-macro tables for brevity.

* Fix a correctness TODO in the register allocator.  This had no effect on
spec2k code generation in master or in this CL, so we were probably just lucky.

* Made some much-needed s/Dest->getType()/Ty/ changes for brevity, in the target
lowering sections that needed other changes.

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

[Jim Stichnoth, 2015-11-09 18:45:35]

https://codereview.chromium.org/1427973003/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1427973003/diff/20001/Makefile.standalone#new...
Makefile.standalone:428: check: check-lit check-unit check-xtest check-spec
On 2015/11/09 16:23:29, John wrote:
> please, don't: check-spec takes a long time in ARM, and it would be nice if I
> could keep
> 
> make check
> 
> for everything else but spec. :)

Done.

https://codereview.chromium.org/1427973003/diff/20001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/1427973003/diff/20001/src/IceInstX8632.def#ne...
src/IceInstX8632.def:58: X(Reg_ah, 4, "ah", Reg_eax, 1,0,0,0, 1,0,0,0,1, 0,
0,0,0,0,1,                \
On 2015/11/09 16:23:29, John wrote:
> so this is what turns these registers on? (no pun intended.)

That's right, the previous CL put the framework in place, and this just flips it
on for i8 variables.  Note that these registers remain disabled for x86-64
register allocation, though they can still be used if accessed by name.

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

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:52: enum RegClassARM32 : uint8_t { RCARM32_NUM =
RC_Target };
On 2015/11/09 16:23:29, John wrote:
> This belongs in the IceRegistersARM32.h file, right?

Done (for MIPS too).

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:81: getRegistersForClass(const Variable *Var) const
override {
On 2015/11/09 16:23:29, John wrote:
> This method should be renamed to, e.g., getRegistersForVariable.

Done.

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86RegClass.h (right):

https://codereview.chromium.org/1427973003/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86RegClass.h:25: RCX86_Is64To8 = RC_Target,
On 2015/11/09 16:23:30, John wrote:
> optinal: comment each class?

Done.

[Jim Stichnoth, 2015-11-09 19:38:46]

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