Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t, particularly so that -1 (or negative values in general) could be used as a NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned sources like enum values and bitvector positions.  This results in a number of clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered comparison operators to help catch errors where particular register number orderings are assumed (as opposed to orderings of the encoded register values).

In addition, it creates a RegNumBitVector wrapper class that makes it much cleaner to do range-based for loops over bit vectors that represent RegNumT sets.

BUG= none
R=eholk@chromium.org, jpp@chromium.org

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

[Jim Stichnoth, 2016-02-08 00:28:51]

Description was changed from

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

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

to

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish between
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors assuming particular register number
orderings (as opposed to orderings of the encoded register values).

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

[Jim Stichnoth, 2016-02-08 00:28:51]

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

[Jim Stichnoth, 2016-02-08 06:37:15]

Description was changed from

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish between
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors assuming particular register number
orderings (as opposed to orderings of the encoded register values).

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

to

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors assuming particular register number
orderings (as opposed to orderings of the encoded register values).

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

[Jim Stichnoth, 2016-02-08 06:38:10]

Description was changed from

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors assuming particular register number
orderings (as opposed to orderings of the encoded register values).

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

to

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors where particular register number
orderings are assumed (as opposed to orderings of the encoded register values).

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

[Eric Holk, 2016-02-08 19:37:10]

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:201: }
Optional, but would it make sense to turn getI64PairFirstGPRNum and
getEncodedGPR into member functions of something like RegNumARM32T?

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h#newcode192
src/IceDefs.h:192: RegNumT(BaseType Value) : Value(Value) {}
Should this check that Value is a legal register value?

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h#newcode200
src/IceDefs.h:200: static RegNumT fixme(BaseType Value) { return RegNumT(Value);
}
Is the idea that eventually we won't do any conversion to and from integers and
then fixme will go away?

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:957: i = Iter.RegMaskUnfiltered.find_next(i)) {
A lot of the changes in this file seem kind of independent of the change to
RegNumT. Am I missing the connection, or would it be better to consider
splitting into two CLs?

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:108: assert(unsigned(RegNum) < Reg_NUM);
Isn't this the sort of thing fixme is supposed to flag?

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersMIPS32.h
File src/IceRegistersMIPS32.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersMIPS32....
src/IceRegistersMIPS32.h:57: assert(int(Reg_GPR_First) <= int(RegNum));
Again, is this the sort of place fixme should be used?

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:84: assert(int(Encoded_X87ST_First) <= int(X87RegNum));
Why cast both of these to int? Where they both unsigned to start with? If so,
why not leave them that way?

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

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1901: for (uint32_t i = 0; i < RegARM32::Reg_NUM;
++i) {
Would something like a Register iterator make sense, so that we could do
something like `for(RegNumT i : RegARM32::AllRegisters)`?

It's probably more code than it's worth for just this one instance...

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:294: assert(unsigned(RegNum) <
RegMIPS32::Reg_NUM);
Should there be a fixme somewhere around here?

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:308: assert(unsigned(RegNum) <
PhysicalRegisters[Ty].size());
fixme?

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:426: // RegHi = RegNumT::fixme(RegMIPS32::Reg_A0
+ NumGPRRegsUsed);
Could we just delete this line, since it's commented out?

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:304: static inline void
assertValidRegNum(RegNumT RegNum) {
This is the third time I've seen this function. It seems like we should move it
to a common location.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:383: RegisterSet::base,
Was this a bug previously? This change looks like it has different behavior than
before.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:755: return getGprForType(Ty,
GprForArgNum[ArgNum]);
I love how many casts this change is getting rid of!

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:1288: RegNumT RegNum = RegNumT(i);
Using auto would let us get rid of at least one RegNum.

[Jim Stichnoth, 2016-02-09 19:33:40]

Made some changes based on John's suggestions, and tried to address Eric's
comments.

I have another idea about how to add more safety/usability to the RegNumT class,
so feel free to hold off reviewing for now.

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:201: }
On 2016/02/08 19:37:09, Eric Holk wrote:
> Optional, but would it make sense to turn getI64PairFirstGPRNum and
> getEncodedGPR into member functions of something like RegNumARM32T?

Those functions are supposed to be GPRRegister which are encoded register
values.  I think that's already the right representation.  However, maybe this
function should be returning GPRRegister instead of IValueT, but that's probably
beyond the scope of this CL.

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h#newcode192
src/IceDefs.h:192: RegNumT(BaseType Value) : Value(Value) {}
On 2016/02/08 19:37:09, Eric Holk wrote:
> Should this check that Value is a legal register value?

I don't really think it's possible to check for validity, since this is a
high-level ICE construct and the specific values supplied will be
target-specific.  Or did you have another idea?

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h#newcode200
src/IceDefs.h:200: static RegNumT fixme(BaseType Value) { return RegNumT(Value);
}
On 2016/02/08 19:37:09, Eric Holk wrote:
> Is the idea that eventually we won't do any conversion to and from integers
and
> then fixme will go away?

Yes - added a TODO to remove the method when all uses are fixed.

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:957: i = Iter.RegMaskUnfiltered.find_next(i)) {
On 2016/02/08 19:37:09, Eric Holk wrote:
> A lot of the changes in this file seem kind of independent of the change to
> RegNumT. Am I missing the connection, or would it be better to consider
> splitting into two CLs?

True - while tracking down all the changes, I also took the opportunity to do
proper iteration over bitvector entries, including fixing int32_t==>int to match
the llvm::BitVector API.

I can revert that if you like, but TBH I'd rather just roll it in here. :)

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:108: assert(unsigned(RegNum) < Reg_NUM);
On 2016/02/08 19:37:10, Eric Holk wrote:
> Isn't this the sort of thing fixme is supposed to flag?

fixme() is really to flag inappropriate arithmetic operations on enum values. 
Since RegNumT is high-level ICE without knowledge of target-specific register
values, the check ends up being here.

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersMIPS32.h
File src/IceRegistersMIPS32.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersMIPS32....
src/IceRegistersMIPS32.h:57: assert(int(Reg_GPR_First) <= int(RegNum));
On 2016/02/08 19:37:10, Eric Holk wrote:
> Again, is this the sort of place fixme should be used?

This would be especially hard to represent in RegNumT, I think, because it's not
only dependent on the target, but also specifically about whether the register
value represents a GPR or not.

Probably the right approach here would be to use an x-macro table to validate
the GPR status and hold the encoded value.  Since Reed is trying to follow the
ARM CL trail, it will probably end up that way in time.

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:84: assert(int(Encoded_X87ST_First) <= int(X87RegNum));
On 2016/02/08 19:37:10, Eric Holk wrote:
> Why cast both of these to int? Where they both unsigned to start with? If so,
> why not leave them that way?

The int() cast is because otherwise g++ warns that the unsigned comparison
against 0 will always succeed, and -Werror makes the build fail. :(

The x87 stuff isn't really part of the RegNumT enum space.  It's kind of a hack
because we basically use the x87 floating point stack purely for the x86-32
function call ABI which says that FP values are returned in st(0).

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

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1901: for (uint32_t i = 0; i < RegARM32::Reg_NUM;
++i) {
On 2016/02/08 19:37:10, Eric Holk wrote:
> Would something like a Register iterator make sense, so that we could do
> something like `for(RegNumT i : RegARM32::AllRegisters)`?
> 
> It's probably more code than it's worth for just this one instance...

Good idea for future work. :)

I had thought about making AllRegisters a subclass of RegNumT and then this kind
of iteration would fit right in, but John is concerned about upcast/downcast
issues.

But along those lines, I was thinking of wrapping llvm::SmallBitVector so that
find_first() and find_next() could directly return usable RegNumT values...

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:294: assert(unsigned(RegNum) <
RegMIPS32::Reg_NUM);
On 2016/02/08 19:37:10, Eric Holk wrote:
> Should there be a fixme somewhere around here?

Probably not, given this is basically a bounds check.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:308: assert(unsigned(RegNum) <
PhysicalRegisters[Ty].size());
On 2016/02/08 19:37:10, Eric Holk wrote:
> fixme?

same as above I think

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:426: // RegHi = RegNumT::fixme(RegMIPS32::Reg_A0
+ NumGPRRegsUsed);
On 2016/02/08 19:37:10, Eric Holk wrote:
> Could we just delete this line, since it's commented out?

I'll leave that to Reed, he may be holding the comment for future enabling.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:304: static inline void
assertValidRegNum(RegNumT RegNum) {
On 2016/02/08 19:37:10, Eric Holk wrote:
> This is the third time I've seen this function. It seems like we should move
it
> to a common location.

That could be tricky given that the upper bound is target-specific.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:383: RegisterSet::base,
On 2016/02/08 19:37:10, Eric Holk wrote:
> Was this a bug previously? This change looks like it has different behavior
than
> before.

Yes, it was a bug, but shockingly it didn't affect at least the spec2k
translation.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:755: return getGprForType(Ty,
GprForArgNum[ArgNum]);
On 2016/02/08 19:37:10, Eric Holk wrote:
> I love how many casts this change is getting rid of!

Acknowledged.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:1288: RegNumT RegNum = RegNumT(i);
On 2016/02/08 19:37:10, Eric Holk wrote:
> Using auto would let us get rid of at least one RegNum.

Yeah, I was thinking of making another pass and changing most of these to auto.

[Karl, 2016-02-09 21:52:15]

Nearly Good To Me (I didn't see anything wrong, but there are a lot of changes).

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:201: }
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:09, Eric Holk wrote:
> > Optional, but would it make sense to turn getI64PairFirstGPRNum and
> > getEncodedGPR into member functions of something like RegNumARM32T?
> 
> Those functions are supposed to be GPRRegister which are encoded register
> values.  I think that's already the right representation.  However, maybe this
> function should be returning GPRRegister instead of IValueT, but that's
probably
> beyond the scope of this CL.

Be careful when removing IValueT. It was explicitly defined to guarantee that we
can bit-union in bits of an ARM instruction.

[Jim Stichnoth, 2016-02-10 01:06:35]

Description was changed from

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors where particular register number
orderings are assumed (as opposed to orderings of the encoded register values).

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

to

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors where particular register number
orderings are assumed (as opposed to orderings of the encoded register values).

In addition, it creates a RegNumBitVector wrapper class that makes it much
cleaner to do range-based for loops over bit vectors that represent RegNumT
sets.

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

[Jim Stichnoth, 2016-02-10 01:09:21]

PTAL.

The latest patchsets make a few improvements:

1. Allow the target to supply an upper bound on valid RegNumT values, for
asserting validity in the RegNumT ctor.

2. Add RegNumBitVector adapter class to make range-based for loops over register
bitvectors cleaner.

3. Use "auto" instead of RegNumT where appropriate.

[Eric Holk, 2016-02-10 01:11:31]

lgtm

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:201: }
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:09, Eric Holk wrote:
> > Optional, but would it make sense to turn getI64PairFirstGPRNum and
> > getEncodedGPR into member functions of something like RegNumARM32T?
> 
> Those functions are supposed to be GPRRegister which are encoded register
> values.  I think that's already the right representation.  However, maybe this
> function should be returning GPRRegister instead of IValueT, but that's
probably
> beyond the scope of this CL.

Sounds good.

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h#newcode192
src/IceDefs.h:192: RegNumT(BaseType Value) : Value(Value) {}
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:09, Eric Holk wrote:
> > Should this check that Value is a legal register value?
> 
> I don't really think it's possible to check for validity, since this is a
> high-level ICE construct and the specific values supplied will be
> target-specific.  Or did you have another idea?

Hmm, good point. I guess you'd have to do something like have a RegNumARM32T,
RegNumX86T, etc and have each of them handle validating. You could do it with a
protected constructor to prevent rogue register numbers from being created, but
it might make it hard to create new registers variables and such in the
target-independent code.

https://codereview.chromium.org/1676123002/diff/40001/src/IceDefs.h#newcode200
src/IceDefs.h:200: static RegNumT fixme(BaseType Value) { return RegNumT(Value);
}
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:09, Eric Holk wrote:
> > Is the idea that eventually we won't do any conversion to and from integers
> and
> > then fixme will go away?
> 
> Yes - added a TODO to remove the method when all uses are fixed.

Sounds good.

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:957: i = Iter.RegMaskUnfiltered.find_next(i)) {
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:09, Eric Holk wrote:
> > A lot of the changes in this file seem kind of independent of the change to
> > RegNumT. Am I missing the connection, or would it be better to consider
> > splitting into two CLs?
> 
> True - while tracking down all the changes, I also took the opportunity to do
> proper iteration over bitvector entries, including fixing int32_t==>int to
match
> the llvm::BitVector API.
> 
> I can revert that if you like, but TBH I'd rather just roll it in here. :)

Yeah, that's fine with me. It's a pain to split up CLs!

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:108: assert(unsigned(RegNum) < Reg_NUM);
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Isn't this the sort of thing fixme is supposed to flag?
> 
> fixme() is really to flag inappropriate arithmetic operations on enum values. 
> Since RegNumT is high-level ICE without knowledge of target-specific register
> values, the check ends up being here.

Sounds good.

Yeah, I guess this isn't really making any decisions based on register ordering
(like I did to determine QtoS registers), so there's no harm in this comparison.

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersMIPS32.h
File src/IceRegistersMIPS32.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersMIPS32....
src/IceRegistersMIPS32.h:57: assert(int(Reg_GPR_First) <= int(RegNum));
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Again, is this the sort of place fixme should be used?
> 
> This would be especially hard to represent in RegNumT, I think, because it's
not
> only dependent on the target, but also specifically about whether the register
> value represents a GPR or not.
> 
> Probably the right approach here would be to use an x-macro table to validate
> the GPR status and hold the encoded value.  Since Reed is trying to follow the
> ARM CL trail, it will probably end up that way in time.

That makes sense.

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:84: assert(int(Encoded_X87ST_First) <= int(X87RegNum));
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Why cast both of these to int? Where they both unsigned to start with? If
so,
> > why not leave them that way?
> 
> The int() cast is because otherwise g++ warns that the unsigned comparison
> against 0 will always succeed, and -Werror makes the build fail. :(
> 
> The x87 stuff isn't really part of the RegNumT enum space.  It's kind of a
hack
> because we basically use the x87 floating point stack purely for the x86-32
> function call ABI which says that FP values are returned in st(0).

Ah, okay. That makes sense.

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

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1901: for (uint32_t i = 0; i < RegARM32::Reg_NUM;
++i) {
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Would something like a Register iterator make sense, so that we could do
> > something like `for(RegNumT i : RegARM32::AllRegisters)`?
> > 
> > It's probably more code than it's worth for just this one instance...
> 
> Good idea for future work. :)
> 
> I had thought about making AllRegisters a subclass of RegNumT and then this
kind
> of iteration would fit right in, but John is concerned about upcast/downcast
> issues.
> 
> But along those lines, I was thinking of wrapping llvm::SmallBitVector so that
> find_first() and find_next() could directly return usable RegNumT values...

Cool :)

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:294: assert(unsigned(RegNum) <
RegMIPS32::Reg_NUM);
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Should there be a fixme somewhere around here?
> 
> Probably not, given this is basically a bounds check.

Good point.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:308: assert(unsigned(RegNum) <
PhysicalRegisters[Ty].size());
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > fixme?
> 
> same as above I think

I think so too. Plus, the code is in an assert, so I think we can be a little
more forgiving.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:426: // RegHi = RegNumT::fixme(RegMIPS32::Reg_A0
+ NumGPRRegsUsed);
On 2016/02/09 19:33:39, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Could we just delete this line, since it's commented out?
> 
> I'll leave that to Reed, he may be holding the comment for future enabling.

Ok.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:304: static inline void
assertValidRegNum(RegNumT RegNum) {
On 2016/02/09 19:33:40, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > This is the third time I've seen this function. It seems like we should move
> it
> > to a common location.
> 
> That could be tricky given that the upper bound is target-specific.

Ah, right. It does make me think there might be another way to factor this, but
maybe you've tried that in some of your later changes.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:383: RegisterSet::base,
On 2016/02/09 19:33:40, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Was this a bug previously? This change looks like it has different behavior
> than
> > before.
> 
> Yes, it was a bug, but shockingly it didn't affect at least the spec2k
> translation.

Huh. Good thing we caught it.

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1676123002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:1288: RegNumT RegNum = RegNumT(i);
On 2016/02/09 19:33:40, stichnot wrote:
> On 2016/02/08 19:37:10, Eric Holk wrote:
> > Using auto would let us get rid of at least one RegNum.
> 
> Yeah, I was thinking of making another pass and changing most of these to
auto.

Acknowledged.

https://codereview.chromium.org/1676123002/diff/140001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceCfgNode.cpp#new...
src/IceCfgNode.cpp:342: auto RegNum2 = Var2->getRegNum();
Optional, but these could probably be const as well. Likewise with most of the
other autos.

[Jim Stichnoth, 2016-02-10 15:31:57]

https://codereview.chromium.org/1676123002/diff/140001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceCfgNode.cpp#new...
src/IceCfgNode.cpp:342: auto RegNum2 = Var2->getRegNum();
On 2016/02/10 01:11:31, Eric Holk wrote:
> Optional, but these could probably be const as well. Likewise with most of the
> other autos.

Done, here and elsewhere.

There are a number of other nearby opportunities for adding const, but that's
probably better for a separate CL.

[John, 2016-02-10 16:01:51]

lgtm

commented on the wrong patch, but the comments still hold.

https://codereview.chromium.org/1676123002/diff/140001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceAssemblerARM32....
src/IceAssemblerARM32.cpp:588: RegARM32::getRegName(RegNumT::fixme(Reg)) +
this does not look like a "fixme" issue -- reg is Reg_pc.

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:441: // Define NoRegister as an enum value so that it can be
used as an argument
nice. :)

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:475: class RegNumBitVector {
This is an iterator, so maybe just rename it to RegNumBitVectorIter(ator)?

https://codereview.chromium.org/1676123002/diff/140001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceRegAlloc.cpp#ne...
src/IceRegAlloc.cpp:427: // TODO(stichnot): Randomize instead of find_first().
update the comment.

https://codereview.chromium.org/1676123002/diff/140001/src/IceRegAlloc.cpp#ne...
src/IceRegAlloc.cpp:621: for (RegNumT RegAlias : RegNumBitVector(Aliases)) {
just curious why this is not auto, but everywhere else you have auto foo = ...

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

https://codereview.chromium.org/1676123002/diff/140001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664Traits.h:71: static constexpr auto StackPtr =
RegX8664::Reg_rsp;
I would not use auto in class members, and global variables. I tend to think of
these as if they were method parameters -- and therefore you can't use auto in
them.

[Jim Stichnoth, 2016-02-10 17:47:20]

https://codereview.chromium.org/1676123002/diff/140001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceAssemblerARM32....
src/IceAssemblerARM32.cpp:588: RegARM32::getRegName(RegNumT::fixme(Reg)) +
On 2016/02/10 16:01:51, John wrote:
> this does not look like a "fixme" issue -- reg is Reg_pc.

Done.

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:441: // Define NoRegister as an enum value so that it can be
used as an argument
On 2016/02/10 16:01:51, John wrote:
> nice. :)

True, though that inhibits certain uses of "auto", e.g.:
  auto RegNum = RegNumT::NoRegister;
when we want RegNum to have type RegNumT.

I was thinking of something like:

  enum { NoRegisterValue = std::numeric_limits<BaseType>::max() };
  const static RegNumT NoRegister /* = NoRegisterValue */ ;

but I am le tired

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:475: class RegNumBitVector {
On 2016/02/10 16:01:51, John wrote:
> This is an iterator, so maybe just rename it to RegNumBitVectorIter(ator)?

I was trying to control the identifier length a bit. :)

I will try on "RegNumBVIter", I think that captures everything while being
compact.

BTW, I really wanted something like this:

  template <class T = llvm::SmallBitVector> class RegNumBVIter { ... }

because why not allow it for llvm::BitVector as well, but then you end up with
the clumsy

  for (RegNumT RegNum : RegNumBVIter<>(Mask)) { ... }

instead.

https://codereview.chromium.org/1676123002/diff/140001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceRegAlloc.cpp#ne...
src/IceRegAlloc.cpp:427: // TODO(stichnot): Randomize instead of find_first().
On 2016/02/10 16:01:51, John wrote:
> update the comment.

Done.

https://codereview.chromium.org/1676123002/diff/140001/src/IceRegAlloc.cpp#ne...
src/IceRegAlloc.cpp:621: for (RegNumT RegAlias : RegNumBitVector(Aliases)) {
On 2016/02/10 16:01:51, John wrote:
> just curious why this is not auto, but everywhere else you have auto foo = ...

I prefer being more explicit with range-based for loops.  (perhaps a JF-ism)

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

https://codereview.chromium.org/1676123002/diff/140001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664Traits.h:71: static constexpr auto StackPtr =
RegX8664::Reg_rsp;
On 2016/02/10 16:01:51, John wrote:
> I would not use auto in class members, and global variables. I tend to think
of
> these as if they were method parameters -- and therefore you can't use auto in
> them.

Done.

This change made it consistent with the use of "auto" in the X8632 file, so I
changed both to be explicit.

[Jim Stichnoth, 2016-02-10 18:47:30]

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1676123002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:441: // Define NoRegister as an enum value so that it can be
used as an argument
On 2016/02/10 17:47:20, stichnot wrote:
> On 2016/02/10 16:01:51, John wrote:
> > nice. :)
> 
> True, though that inhibits certain uses of "auto", e.g.:
>   auto RegNum = RegNumT::NoRegister;
> when we want RegNum to have type RegNumT.
> 
> I was thinking of something like:
> 
>   enum { NoRegisterValue = std::numeric_limits<BaseType>::max() };
>   const static RegNumT NoRegister /* = NoRegisterValue */ ;
> 
> but I am le tired

I am now le refreshed, and realize the horror of the compiler error messages
that propagate from "auto RegNum = RegNumT::NoRegister;", so I went ahead and
made that change.

[Jim Stichnoth, 2016-02-10 19:20:53]

Description was changed from

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors where particular register number
orderings are assumed (as opposed to orderings of the encoded register values).

In addition, it creates a RegNumBitVector wrapper class that makes it much
cleaner to do range-based for loops over bit vectors that represent RegNumT
sets.

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

to

==========
Subzero: Use a proper RegNumT type instead of int32_t/SizeT.

Originally, register numbers were represented explicitly as int32_t,
particularly so that -1 (or negative values in general) could be used as a
NoRegister sentinel value.  This created two problems:

1. It would be better to use a unique name for the type, to distinguish from
other explicit int32_t uses such as stack offsets.

2. Apart from NoRegister, register number values ultimately come from unsigned
sources like enum values and bitvector positions.  This results in a number of
clumsy casts to remove compiler warnings.

This creates a simple RegNumT class to manage this.  It also deletes ordered
comparison operators to help catch errors where particular register number
orderings are assumed (as opposed to orderings of the encoded register values).

In addition, it creates a RegNumBitVector wrapper class that makes it much
cleaner to do range-based for loops over bit vectors that represent RegNumT
sets.

BUG= none
R=eholk@chromium.org, jpp@chromium.org

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

[Jim Stichnoth, 2016-02-10 19:20:55]

Committed patchset #11 (id:200001) manually as
8aa396610b7baf728631a43ea16ad3d13e38397a (presubmit successful).