Subzero: Prototype to make use of RegNumT::No Register more concise

Subzero: Avoid explicit references to RegNumT sentinel value.

There are many occurrences of if (RegNum == RegNumT::NoRegister).

This patch eliminates NoRegister and provides a simpler mechanism for declaring and testing RegNumT values to see if they are undefined.


BUG= none
R=stichnot@chromium.org

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

[rkotlerimgtec, 2016-02-12 01:32:14]

Description was changed from

==========
cleanup noregister


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


mistaken merge


Merge branch 'master' of
https://chromium.googlesource.com/native_client/pnacl-subzero


move doxypypy

BUG=
==========

to

==========
There are many occurrences of if (RegNum == RegNumT::NoRegister). This change
would allow
if (RegNum::noRegister()) which is more concise and also
makes more lines fall under the 80 character limit in addition.

I can change all the Mips occurrences if this is okay.

BUG=
==========

[rkotlerimgtec, 2016-02-12 01:32:15]

rkotlerimgtec@gmail.com changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2016-02-12 02:20:15]

https://codereview.chromium.org/1691193002/diff/1/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1691193002/diff/1/src/IceOperand.h#newcode446
src/IceOperand.h:446: bool noRegister() {return Value == NoRegisterValue;}
This seems OK.  I might change the name to something like "isNoReg" or
"isDefined" or "isSet" or "hasValue", to make it more clear that it's a
predicate function.

In any case, the method should be marked const.

A separate idea is to change all of these:
  auto RegNum = RegNumT::NoRegister;
to:
  RegNumT RegNum;

because of the RegNumT ctor.  Together, the somewhat clumsy explicit references
to NoRegister would be greatly reduced.  I'm not totally sold on this idea
though, because RegNumT looks an awful lot like a scalar type and we usually
need to explicitly initialize scalar types.

[rkotlerimgtec, 2016-02-12 03:00:16]

How do we want to handle making the related usage changes? Do you want me to
just make the Mips changes? I can make them all if you want.

https://codereview.chromium.org/1691193002/diff/1/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1691193002/diff/1/src/IceOperand.h#newcode446
src/IceOperand.h:446: bool noRegister() {return Value == NoRegisterValue;}
On 2016/02/12 02:20:15, stichnot wrote:
> This seems OK.  I might change the name to something like "isNoReg" or
> "isDefined" or "isSet" or "hasValue", to make it more clear that it's a
> predicate function.
> 
> In any case, the method should be marked const.
> 
> A separate idea is to change all of these:
>   auto RegNum = RegNumT::NoRegister;
> to:
>   RegNumT RegNum;
> 
> because of the RegNumT ctor.  Together, the somewhat clumsy explicit
references
> to NoRegister would be greatly reduced.  I'm not totally sold on this idea
> though, because RegNumT looks an awful lot like a scalar type and we usually
> need to explicitly initialize scalar types.

I've made the change to the predicate.

I think it's a good idea to just let the constructor take care of
initializing variables of type RegNumT. I'm usually for being more
concise. I think in general we want to eliminate all uses of int
and such for things that are not fundamentally numbers (a register number is not
really a number so in that case it should be of some
class type with an appropriate constructor).

[Jim Stichnoth, 2016-02-12 03:16:52]

Please make the changes everywhere, not just the mips code, and be sure to check
with "make presubmit".

https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h#newcod...
src/IceOperand.h:446: bool hasValue() const {return Value == NoRegisterValue;}
You really need "return Value != NoRegisterValue;" to convey the meaning of
hasValue(), which is the opposite of your original noRegister() proposal.

[rkotlerimgtec, 2016-02-12 03:22:50]

I will make the appropriate code change !=  for hasValue

Do we want to make your proposed changed for the object creation?

auto RegNum = RegNumT::NoRegister;
to:
  RegNumT RegNum;


Or should that be in a separate change?


On Thu, Feb 11, 2016 at 7:16 PM, <stichnot@chromium.org> wrote:

> Please make the changes everywhere, not just the mips code, and be sure to
check
> with "make presubmit".
>
> https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h
> File src/IceOperand.h (right):
>
https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h#newcod...
> src/IceOperand.h:446
<https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h#newcod...>:
bool hasValue() const {return Value ==
> NoRegisterValue;}
> You really need "return Value != NoRegisterValue;" to convey the meaning
> of hasValue(), which is the opposite of your original noRegister()
> proposal.
> https://codereview.chromium.org/1691193002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[rkotlerimgtec, 2016-02-12 03:24:53]

How about hasNoValue ?

Otherwise everywhere we will be checking !Reg.hasValue where now we have
Reg == RegNumT::NoRegisterValue

On Thu, Feb 11, 2016 at 7:22 PM, reed kotler <rkotlerimgtec@gmail.com>
wrote:

> I will make the appropriate code change !=  for hasValue
>
> Do we want to make your proposed changed for the object creation?
>
> auto RegNum = RegNumT::NoRegister;
> to:
>   RegNumT RegNum;
>
>
> Or should that be in a separate change?
>
>
> On Thu, Feb 11, 2016 at 7:16 PM, <stichnot@chromium.org> wrote:
>
>> Please make the changes everywhere, not just the mips code, and be sure to
check
>> with "make presubmit".
>>
>> https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h
>> File src/IceOperand.h (right):
>>
https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h#newcod...
>> src/IceOperand.h:446
<https://codereview.chromium.org/1691193002/diff/20001/src/IceOperand.h#newcod...>:
bool hasValue() const {return Value ==
>> NoRegisterValue;}
>> You really need "return Value != NoRegisterValue;" to convey the meaning
>> of hasValue(), which is the opposite of your original noRegister()
>> proposal.
>> https://codereview.chromium.org/1691193002/
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[rkotlerimgtec, 2016-02-12 03:34:11]

I kind of like to have the pair hasValue and hasNoValue

[Jim Stichnoth, 2016-02-12 03:38:36]

On 2016/02/12 03:22:50, rkotlerimgtec wrote:
> I will make the appropriate code change !=  for hasValue

Thanks, it looks good.

> 
> Do we want to make your proposed changed for the object creation?
> 
> auto RegNum = RegNumT::NoRegister;
> to:
>   RegNumT RegNum;
> 
> 
> Or should that be in a separate change?

Internally, you may want to separate the changes into separate local git commits
so that it's easier to revert the latter part if it for some reason turns out to
be disagreeable.  Otherwise a single "git cl upload" is just fine.

I have a suspicion that this would remove the need for all explicit uses of
NoRegister, so that static member could be removed and NoRegisterValue could be
made private.

[rkotlerimgtec, 2016-02-12 03:41:19]

Ok. I will separate out the two changes internally.

If there are no more uses of the constant, i will make it private.


On Thu, Feb 11, 2016 at 7:38 PM, <stichnot@chromium.org> wrote:

> On 2016/02/12 03:22:50, rkotlerimgtec wrote:
> > I will make the appropriate code change !=  for hasValue
>
> Thanks, it looks good.
>
> >
> > Do we want to make your proposed changed for the object creation?
> >
> > auto RegNum = RegNumT::NoRegister;
> > to:
> >   RegNumT RegNum;
> >
> >
> > Or should that be in a separate change?
>
> Internally, you may want to separate the changes into separate local git
commits
> so that it's easier to revert the latter part if it for some reason turns out
to
> be disagreeable.  Otherwise a single "git cl upload" is just fine.
>
> I have a suspicion that this would remove the need for all explicit uses of
> NoRegister, so that static member could be removed and NoRegisterValue could
be
> made private.
> https://codereview.chromium.org/1691193002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[rkotlerimgtec, 2016-02-12 05:17:17]

I decided to do the changes in several commits; this first being to add the
hasValue() and hasNoValue() methods.

I will work on the other separately while this one is being reviewed.

[Jim Stichnoth, 2016-02-12 18:29:54]

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:204: V->RegNum = NewRegNum.hasNoValue() ? RegNum : NewRegNum;
NewRegNum.hasValue() ? NewRegNum : RegNum

similar to how
  if (c) a(); else b();
is preferred over
  if (!c) b(); else a();

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.h#newcod...
src/IceOperand.h:446: bool hasNoValue() const { return !hasValue(); }
Nit: I would reverse the order of these two definitions.

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:6090: if (RegNum.hasNoValue())
if (RegNum.hasValue())
  Reg->setRegNum(RegNum);
else
  Reg->setMustHaveReg();

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:363: if (RegNum.hasNoValue())
if (RegNum.hasValue()) ...
as in the ARM file

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1256: if (RegNum.hasNoValue())
if (RegNum.hasValue()) ...

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2494:
(Traits::getRegisterForXmmArgNum(XmmArgs.size()).hasValue())) {
I would remove the outer level of parentheses around the hasValue() expression,
here and the next two instances below.

[rkotlerimgtec, 2016-02-12 22:02:01]

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:204: V->RegNum = NewRegNum.hasNoValue() ? RegNum : NewRegNum;
On 2016/02/12 18:29:54, stichnot wrote:
> NewRegNum.hasValue() ? NewRegNum : RegNum
> 
> similar to how
>   if (c) a(); else b();
> is preferred over
>   if (!c) b(); else a();

Done.

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1691193002/diff/80001/src/IceOperand.h#newcod...
src/IceOperand.h:446: bool hasNoValue() const { return !hasValue(); }
On 2016/02/12 18:29:54, stichnot wrote:
> Nit: I would reverse the order of these two definitions.

Done.

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:6090: if (RegNum.hasNoValue())
On 2016/02/12 18:29:54, stichnot wrote:
> if (RegNum.hasValue())
>   Reg->setRegNum(RegNum);
> else
>   Reg->setMustHaveReg();

Done.

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:363: if (RegNum.hasNoValue())
On 2016/02/12 18:29:54, stichnot wrote:
> if (RegNum.hasValue()) ...
> as in the ARM file

Done.

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1256: if (RegNum.hasNoValue())
On 2016/02/12 18:29:54, stichnot wrote:
> if (RegNum.hasValue()) ...

Done.

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2494:
(Traits::getRegisterForXmmArgNum(XmmArgs.size()).hasValue())) {
On 2016/02/12 18:29:54, stichnot wrote:
> I would remove the outer level of parentheses around the hasValue()
expression,
> here and the next two instances below.

Done.

[Jim Stichnoth, 2016-02-12 23:14:24]

Were you planning to do the other part in this CL, or a separate CL?

Also, the Description in this CL needs to be made more git-compliant.

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:6090: if (RegNum.hasNoValue())
On 2016/02/12 22:02:01, rkotlerimgtec wrote:
> On 2016/02/12 18:29:54, stichnot wrote:
> > if (RegNum.hasValue())
> >   Reg->setRegNum(RegNum);
> > else
> >   Reg->setMustHaveReg();
> 
> Done.

Doesn't look done yet...

[rkotlerimgtec, 2016-02-13 00:56:53]

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

https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:6090: if (RegNum.hasNoValue())
On 2016/02/12 18:29:54, stichnot wrote:
> if (RegNum.hasValue())
>   Reg->setRegNum(RegNum);
> else
>   Reg->setMustHaveReg();

Done.

[rkotlerimgtec, 2016-02-13 00:59:13]

On 2016/02/12 23:14:24, stichnot wrote:
> Were you planning to do the other part in this CL, or a separate CL?
> 
> Also, the Description in this CL needs to be made more git-compliant.
> 
>
https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
> File src/IceTargetLoweringARM32.cpp (right):
> 
>
https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
> src/IceTargetLoweringARM32.cpp:6090: if (RegNum.hasNoValue())
> On 2016/02/12 22:02:01, rkotlerimgtec wrote:
> > On 2016/02/12 18:29:54, stichnot wrote:
> > > if (RegNum.hasValue())
> > >   Reg->setRegNum(RegNum);
> > > else
> > >   Reg->setMustHaveReg();
> > 
> > Done.
> 
> Doesn't look done yet...

I've submitted the next patch to eliminate NoRegister from all the code. If we
don't like it then I could delete that patch or resubmit it as a separate cl.

[rkotlerimgtec, 2016-02-13 01:01:20]

Description was changed from

==========
There are many occurrences of if (RegNum == RegNumT::NoRegister). This change
would allow
if (RegNum::noRegister()) which is more concise and also
makes more lines fall under the 80 character limit in addition.

I can change all the Mips occurrences if this is okay.

BUG=
==========

to

==========
There are many occurrences of if (RegNum == RegNumT::NoRegister).

This patch eliminates NoRegister and provides a simpler mechanism for declaring
and testing RegNumT values to see if they are undefined.


BUG=
==========

[rkotlerimgtec, 2016-02-13 01:07:13]

On 2016/02/13 00:59:13, rkotlerimgtec wrote:
> On 2016/02/12 23:14:24, stichnot wrote:
> > Were you planning to do the other part in this CL, or a separate CL?
> > 
> > Also, the Description in this CL needs to be made more git-compliant.
> > 
> >
>
https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
> > File src/IceTargetLoweringARM32.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringAR...
> > src/IceTargetLoweringARM32.cpp:6090: if (RegNum.hasNoValue())
> > On 2016/02/12 22:02:01, rkotlerimgtec wrote:
> > > On 2016/02/12 18:29:54, stichnot wrote:
> > > > if (RegNum.hasValue())
> > > >   Reg->setRegNum(RegNum);
> > > > else
> > > >   Reg->setMustHaveReg();
> > > 
> > > Done.
> > 
> > Doesn't look done yet...
> 
> I've submitted the next patch to eliminate NoRegister from all the code. If we
> don't like it then I could delete that patch or resubmit it as a separate cl.

Possibly I made a merge error here. Am looking at it now.

[rkotlerimgtec, 2016-02-13 01:08:32]

Seems to be okay.

On Fri, Feb 12, 2016 at 5:07 PM, <rkotlerimgtec@gmail.com> wrote:

>
> On 2016/02/13 00:59:13, rkotlerimgtec wrote:
> > On 2016/02/12 23:14:24, stichnot wrote:
> > > Were you planning to do the other part in this CL, or a separate CL?
> > >
> > > Also, the Description in this CL needs to be made more git-compliant.
> > >
> > >
>
>https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringARM32.cpp
> > > File src/IceTargetLoweringARM32.cpp (right):
> > >
> > >
>
>https://codereview.chromium.org/1691193002/diff/80001/src/IceTargetLoweringARM32.cpp#newcode6090
> > > src/IceTargetLoweringARM32.cpp:6090: if (RegNum.hasNoValue())
> > > On 2016/02/12 22:02:01, rkotlerimgtec wrote:
> > > > On 2016/02/12 18:29:54, stichnot wrote:
> > > > > if (RegNum.hasValue())
> > > > >   Reg->setRegNum(RegNum);
> > > > > else
> > > > >   Reg->setMustHaveReg();
> > > >
> > > > Done.
> > >
> > > Doesn't look done yet...
> >
> > I've submitted the next patch to eliminate NoRegister from all the code. If
we
> > don't like it then I could delete that patch or resubmit it as a separate
cl.
>
> Possibly I made a merge error here. Am looking at it now.
> https://codereview.chromium.org/1691193002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2016-02-13 18:58:10]

Thanks!  Just a few more comment corrections...

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:406: /// value NoRegister.  Its public ctor allows direct use
of enum values, such as
..., plus a sentinel value if no register is assigned.

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:707: /// VarsReal.  If NewRegNum!=NoRegister, then that
register assignment is made
If NewRegNum.hasValue(), then ...

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:746: /// RegNum is the allocated register, or NoRegister if it
isn't
/// RegNum is the allocated register (as long as RegNum.hasValue() is true).

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:752: /// RegNum==NoRegister || IsArgument).
RegNum.hasNoValue()

[Jim Stichnoth, 2016-02-13 18:59:24]

Description was changed from

==========
There are many occurrences of if (RegNum == RegNumT::NoRegister).

This patch eliminates NoRegister and provides a simpler mechanism for declaring
and testing RegNumT values to see if they are undefined.


BUG=
==========

to

==========
Subzero: Avoid explicit references to RegNumT sentinel value.

There are many occurrences of if (RegNum == RegNumT::NoRegister).

This patch eliminates NoRegister and provides a simpler mechanism for declaring
and testing RegNumT values to see if they are undefined.


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

[rkotlerimgtec, 2016-02-14 00:04:05]

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:406: /// value NoRegister.  Its public ctor allows direct use
of enum values, such as
On 2016/02/13 18:58:10, stichnot wrote:
> ..., plus a sentinel value if no register is assigned.

Done.

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:707: /// VarsReal.  If NewRegNum!=NoRegister, then that
register assignment is made
On 2016/02/13 18:58:10, stichnot wrote:
> If NewRegNum.hasValue(), then ...

Done.

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:746: /// RegNum is the allocated register, or NoRegister if it
isn't
On 2016/02/13 18:58:10, stichnot wrote:
> /// RegNum is the allocated register (as long as RegNum.hasValue() is true).

Done.

https://codereview.chromium.org/1691193002/diff/180001/src/IceOperand.h#newco...
src/IceOperand.h:752: /// RegNum==NoRegister || IsArgument).
On 2016/02/13 18:58:10, stichnot wrote:
> RegNum.hasNoValue()

Done.

[Jim Stichnoth, 2016-02-16 03:40:41]

LGTM.

Sorry for the delay.

[Jim Stichnoth, 2016-02-16 04:01:41]

Description was changed from

==========
Subzero: Avoid explicit references to RegNumT sentinel value.

There are many occurrences of if (RegNum == RegNumT::NoRegister).

This patch eliminates NoRegister and provides a simpler mechanism for declaring
and testing RegNumT values to see if they are undefined.


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

to

==========
Subzero: Avoid explicit references to RegNumT sentinel value.

There are many occurrences of if (RegNum == RegNumT::NoRegister).

This patch eliminates NoRegister and provides a simpler mechanism for declaring
and testing RegNumT values to see if they are undefined.


BUG= none
R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-02-16 04:01:43]

Committed patchset #11 (id:200001) manually as
5fa0a5f7f730f33e6987527a261de8d833d7585c (presubmit successful).