Subzero: Refactor x86 register definitions to use the alias mechanism.

Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when we really mean ax or al.  This gets rid of the ShiftHack hack.  The one exception is the pinsr instruction which always requires an r32 register even if the memory operand is m8 or m16.

The x86 assembler unit tests are fixed, by not passing a GlobalContext arg to the Assembler ctor.

Many constexpr and "auto *" upgrades are applied.  Sorry for not putting this into a separate CL - a few local fixes got out of hand...

Tested in the following ways:
- "make check-lit" - some .ll CHECK line changes due to register randomization
- "make check-xtest"
- "make check-xtest" with forced filetype=asm (via local .py hack)
- spec2k with all -filetype options
- compare before-and-after spec2k filetype=asm output - a few differences where the correct narrow register is used instead of the full-width register

To do in the next CL:

1. Add new register classes:
  (a) 32-bit GPR truncable to 8-bit (eax, ecx, edx, ebx)
  (b) 16-bit GPR truncable to 8-bit (ax, cx, dx, bx)
  (c) 8-bit truncable from 16/32-bit (al, bl, cl, dl)
  (c) 8-bit "mov"able from ah/bh/ch/dh

2. Enable use of ah/bh/ch/dh for x86-32.

3. Enable use of ah (but skip bh/ch/dh) for x86-64.

4. Statically initialize register tables in the TargetLowering subclass.

BUG= none
R=jpp@chromium.org, kschimpf@google.com

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

[Jim Stichnoth, 2015-10-23 12:11:29]

Description was changed from

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

BUG= none
==========

to

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

The x86 assembler unit tests are broken (won't compile), but that was the case
before this CL because of an Assembler change.

BUG= none
==========

[Jim Stichnoth, 2015-10-23 12:33:46]

Description was changed from

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

The x86 assembler unit tests are broken (won't compile), but that was the case
before this CL because of an Assembler change.

BUG= none
==========

to

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

The x86 assembler unit tests are fixed so that they now compile, but fail on an
assertion at startup.

BUG= none
==========

[Jim Stichnoth, 2015-10-23 12:52:42]

Description was changed from

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

The x86 assembler unit tests are fixed so that they now compile, but fail on an
assertion at startup.

BUG= none
==========

to

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

The x86 assembler unit tests are fixed.

BUG= none
==========

[Jim Stichnoth, 2015-10-26 16:17:15]

Description was changed from

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

The x86 assembler unit tests are fixed.

BUG= none
==========

to

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet
turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when
we really mean ax or al.  The one exception is the pinsr instruction which
always requires an r32 register even if the memory operand is m8 or m16.

The x86 assembler unit tests are fixed.

BUG= none
==========

[Jim Stichnoth, 2015-10-26 19:28:00]

Description was changed from

==========
Subzero: Refactor x86 register definitions to use the alias mechanism.

Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet
turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when
we really mean ax or al.  The one exception is the pinsr instruction which
always requires an r32 register even if the memory operand is m8 or m16.

The x86 assembler unit tests are fixed.

BUG= none
==========

to

==========
Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet
turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when
we really mean ax or al.  This gets rid of the ShiftHack hack.  The one
exception is the pinsr instruction which always requires an r32 register even if
the memory operand is m8 or m16.

The x86 assembler unit tests are fixed, by not passing a GlobalContext arg to
the Assembler ctor.

Many constexpr upgrades are applied.

BUG= none
==========

[Jim Stichnoth, 2015-10-26 21:56:22]

Description was changed from

==========
Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet
turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when
we really mean ax or al.  This gets rid of the ShiftHack hack.  The one
exception is the pinsr instruction which always requires an r32 register even if
the memory operand is m8 or m16.

The x86 assembler unit tests are fixed, by not passing a GlobalContext arg to
the Assembler ctor.

Many constexpr upgrades are applied.

BUG= none
==========

to

==========
Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet
turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when
we really mean ax or al.  This gets rid of the ShiftHack hack.  The one
exception is the pinsr instruction which always requires an r32 register even if
the memory operand is m8 or m16.

The x86 assembler unit tests are fixed, by not passing a GlobalContext arg to
the Assembler ctor.

Many constexpr and "auto *" upgrades are applied.  Sorry for not putting this
into a separate CL - a few local fixes got out of hand...

To do in the next CL:

1. Add new register classes:
  (a) 32-bit GPR truncable to 8-bit (eax, ecx, edx, ebx)
  (b) 16-bit GPR truncable to 8-bit (ax, cx, dx, bx)
  (c) 8-bit truncable from 16/32-bit (al, bl, cl, dl)
  (c) 8-bit "mov"able from ah/bh/ch/dh

2. Enable use of ah/bh/ch/dh for x86-32.

3. Enable use of ah (but skip bh/ch/dh) for x86-64.

4. Statically initialize register tables in the TargetLowering subclass.

BUG= none
==========

[Jim Stichnoth, 2015-10-26 22:36:09]

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

[Jim Stichnoth, 2015-10-26 22:41:44]

Description was changed from

==========
Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet
turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when
we really mean ax or al.  This gets rid of the ShiftHack hack.  The one
exception is the pinsr instruction which always requires an r32 register even if
the memory operand is m8 or m16.

The x86 assembler unit tests are fixed, by not passing a GlobalContext arg to
the Assembler ctor.

Many constexpr and "auto *" upgrades are applied.  Sorry for not putting this
into a separate CL - a few local fixes got out of hand...

To do in the next CL:

1. Add new register classes:
  (a) 32-bit GPR truncable to 8-bit (eax, ecx, edx, ebx)
  (b) 16-bit GPR truncable to 8-bit (ax, cx, dx, bx)
  (c) 8-bit truncable from 16/32-bit (al, bl, cl, dl)
  (c) 8-bit "mov"able from ah/bh/ch/dh

2. Enable use of ah/bh/ch/dh for x86-32.

3. Enable use of ah (but skip bh/ch/dh) for x86-64.

4. Statically initialize register tables in the TargetLowering subclass.

BUG= none
==========

to

==========
Sets the stage for enabling the use of the 8-bit high registers, but doesn't yet
turn it on because more work is needed for correctness.

In the lowering, typing is tightened up so that we don't specify e.g. eax when
we really mean ax or al.  This gets rid of the ShiftHack hack.  The one
exception is the pinsr instruction which always requires an r32 register even if
the memory operand is m8 or m16.

The x86 assembler unit tests are fixed, by not passing a GlobalContext arg to
the Assembler ctor.

Many constexpr and "auto *" upgrades are applied.  Sorry for not putting this
into a separate CL - a few local fixes got out of hand...

Tested in the following ways:
- "make check-lit" - some .ll CHECK line changes due to register randomization
- "make check-xtest"
- "make check-xtest" with forced filetype=asm (via local .py hack)
- spec2k with all -filetype options
- compare before-and-after spec2k filetype=asm output - a few differences where
the correct narrow register is used instead of the full-width register

To do in the next CL:

1. Add new register classes:
  (a) 32-bit GPR truncable to 8-bit (eax, ecx, edx, ebx)
  (b) 16-bit GPR truncable to 8-bit (ax, cx, dx, bx)
  (c) 8-bit truncable from 16/32-bit (al, bl, cl, dl)
  (c) 8-bit "mov"able from ah/bh/ch/dh

2. Enable use of ah/bh/ch/dh for x86-32.

3. Enable use of ah (but skip bh/ch/dh) for x86-64.

4. Statically initialize register tables in the TargetLowering subclass.

BUG= none
==========

[Karl, 2015-10-27 16:13:19]

Otherwise LGTM.

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
File src/IceELFObjectWriter.cpp (right):

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:81: const Elf64_Xword SymTabAlign = ELF64 ? 8 : 4;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:83: ELF64 ? sizeof(Elf64_Sym) : sizeof(Elf32_Sym);
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:221: const Elf64_Xword ShFlags = SHF_ALLOC |
SHF_EXECINSTR;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:330: const Elf64_Xword ShFlags = SHF_ALLOC;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:341: const Elf64_Xword ShFlags = SHF_ALLOC |
SHF_WRITE;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:352: const Elf64_Xword ShFlags = SHF_ALLOC |
SHF_WRITE;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:371: const Elf64_Xword MinAlign = 1;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:376: const uint8_t SymbolBinding = IsExternal ?
STB_GLOBAL : STB_LOCAL;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:425: const Elf64_Off DummySHOffset = 0;
More constexprs?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:575: const uint8_t SymbolType = STT_OBJECT;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceELFObjec...
src/IceELFObjectWriter.cpp:658: const size_t ShdrAlign = ELF64 ? 8 : 4;
constexpr?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceInstARM3...
File src/IceInstARM32.cpp (right):

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceInstARM3...
src/IceInstARM32.cpp:610: const char *NoWidthString = "";
constexprs?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceInstARM3...
src/IceInstARM32.cpp:627: const char *NoWidthString = "";
constexprs?

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceTargetLo...
File src/IceTargetLoweringX8664Traits.h (right):

https://chromiumcodereview.appspot.com/1419903002/diff/250001/src/IceTargetLo...
src/IceTargetLoweringX8664Traits.h:484: const uint32_t Index = (scratch << 0) |
(preserved << 1) | (is8 << 2) |    \
There appears to be patterns in these shifts (is8 << 2) etc.  Should the 2 be a
constexpr is8 << is8Shift?

[Jim Stichnoth, 2015-10-27 19:14:09]

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:81: const Elf64_Xword SymTabAlign = ELF64 ? 8 : 4;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

No because ELF64 is not a constexpr.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:83: ELF64 ? sizeof(Elf64_Sym) : sizeof(Elf32_Sym);
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

No because ELF64 is not a constexpr.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:221: const Elf64_Xword ShFlags = SHF_ALLOC |
SHF_EXECINSTR;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

Done.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:330: const Elf64_Xword ShFlags = SHF_ALLOC;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

Done.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:341: const Elf64_Xword ShFlags = SHF_ALLOC |
SHF_WRITE;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

Done.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:352: const Elf64_Xword ShFlags = SHF_ALLOC |
SHF_WRITE;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

Done.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:371: const Elf64_Xword MinAlign = 1;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

Done.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:376: const uint8_t SymbolBinding = IsExternal ?
STB_GLOBAL : STB_LOCAL;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

no because of IsExternal.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:425: const Elf64_Off DummySHOffset = 0;
On 2015/10/27 16:13:18, Karl wrote:
> More constexprs?

Done.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:575: const uint8_t SymbolType = STT_OBJECT;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

Done.

https://codereview.chromium.org/1419903002/diff/250001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:658: const size_t ShdrAlign = ELF64 ? 8 : 4;
On 2015/10/27 16:13:18, Karl wrote:
> constexpr?

no because of ELF64.

https://codereview.chromium.org/1419903002/diff/250001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1419903002/diff/250001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:610: const char *NoWidthString = "";
On 2015/10/27 16:13:18, Karl wrote:
> constexprs?

C++ compiler says no.

https://codereview.chromium.org/1419903002/diff/250001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:627: const char *NoWidthString = "";
On 2015/10/27 16:13:19, Karl wrote:
> constexprs?

C++ compiler says no.

https://codereview.chromium.org/1419903002/diff/250001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1419903002/diff/250001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664Traits.h:484: const uint32_t Index = (scratch << 0) |
(preserved << 1) | (is8 << 2) |    \
On 2015/10/27 16:13:19, Karl wrote:
> There appears to be patterns in these shifts (is8 << 2) etc.  Should the 2 be
a
> constexpr is8 << is8Shift?

The purpose here was to combine the relevant attributes of the register into a
single value that is used to identify its equivalence class.  I think it's
easier to convey this using the obviously unique constants, rather than using
symbolic constants that aren't obviously unique (unless you look back to their
definitions).

That said, I restructured this so that it conveys the intent without using magic
constants.

[Jim Stichnoth, 2015-10-28 04:12:52]

John, I resurrected byte register representation, ptal.

[John, 2015-10-28 12:48:07]

lgtm

https://codereview.chromium.org/1419903002/diff/310001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:957: typename Traits::ByteRegister>::value ||
you need to revive this

https://codereview.chromium.org/1419903002/diff/310001/src/IceConditionCodesX...
File src/IceConditionCodesX8632.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceConditionCodesX...
src/IceConditionCodesX8632.h:32: #define X(tag, encode, opp, dump, emit) tag
encode,
Not that it matters, but I think tag was more clear. OK as is, though.

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX8664.def#n...
src/IceInstX8664.def:90: REGLIST3(RegX8664, rbx, ebx, bx))				               \
replace these tabs with spaces?

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:649: if (ShiftHack && ShiftReg &&
I don't know if this will work for X8664, but I'll cross that bridge when I get
there (hopefully later this quarter.)
Anyway, this is a major improvement. :)

https://codereview.chromium.org/1419903002/diff/310001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceRegistersX8632....
src/IceRegistersX8632.h:24: class RegX8632 {
RegX8632 is not class. It is really a namespace:
1) It is not meant to be instantiated, but there's nothing preventing me from
creating an object of this type;
2) No non-public static members.

What do you think?

[Jim Stichnoth, 2015-10-28 14:03:55]

https://codereview.chromium.org/1419903002/diff/310001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:957: typename Traits::ByteRegister>::value ||
On 2015/10/28 12:48:06, John wrote:
> you need to revive this

Done.  </shamecube>

https://codereview.chromium.org/1419903002/diff/310001/src/IceConditionCodesX...
File src/IceConditionCodesX8632.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceConditionCodesX...
src/IceConditionCodesX8632.h:32: #define X(tag, encode, opp, dump, emit) tag
encode,
On 2015/10/28 12:48:06, John wrote:
> Not that it matters, but I think tag was more clear. OK as is, though.

I was trying to make all the x-macros consistent with "val" meaning "enum
value".

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX8664.def#n...
src/IceInstX8664.def:90: REGLIST3(RegX8664, rbx, ebx, bx))				               \
On 2015/10/28 12:48:06, John wrote:
> replace these tabs with spaces?

Done.

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:649: if (ShiftHack && ShiftReg &&
On 2015/10/28 12:48:06, John wrote:
> I don't know if this will work for X8664, but I'll cross that bridge when I
get
> there (hopefully later this quarter.)
> Anyway, this is a major improvement. :)

I think this particular thing ought to work for x86-64.

However, I tried hand-running the filetype=asm translation process for one of
the spec2k components using x86-64, and it looks like a number of problems have
crept in (or maybe were there from the start).  :(

https://codereview.chromium.org/1419903002/diff/310001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (left):

https://codereview.chromium.org/1419903002/diff/310001/src/IceRegistersX8632....
src/IceRegistersX8632.h:24: class RegX8632 {
On 2015/10/28 12:48:07, John wrote:
> RegX8632 is not class. It is really a namespace:
> 1) It is not meant to be instantiated, but there's nothing preventing me from
> creating an object of this type;
> 2) No non-public static members.
> 
> What do you think?

I tried changing "class" to "namespace", but hit this:

src/IceTargetLoweringX8632Traits.h:69:30: error: no type named 'RegX8632' in
namespace 'Ice'
  using RegisterSet = ::Ice::RegX8632;
                      ~~~~~~~^

Any ideas?

[Jim Stichnoth, 2015-10-28 16:26:06]

Committed patchset #18 (id:330001) manually as
5bff61c44841990680781892036adb17b3cff0c4 (presubmit successful).