[MIPS] Add thread-pointer to data addressing register list.

[MIPS] Add thread-pointer to data addressing register list.

There is already a rule which prevents modification of thread pointer,
so we can be sure the value inside is correct. Thus, no need to guard
loads and stores via tp.
In addition, the change gets rid of some non-POD static constructors.

The commit queue is not working for NaCl at the moment, so we are committing
with:
NOTRY=true
However, we have run a try job manually, and it passed fine.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=2275
TEST= pnacl/build.sh misc-tools

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=10686

[petarj, 2013-01-15 16:16:54]

This change is in relation with:
https://codereview.chromium.org/11883035/

[Mark Seaborn, 2013-01-15 16:51:36]

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/mo...
File src/trusted/validator_mips/model.h (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/mo...
src/trusted/validator_mips/model.h:112: static RegisterList const
kRegListReserved = RegisterList(kReservedRegsBitmask);
Nit: Can you make the ordering of declarations consistent?

Either:

kReservedRegsBitmask
kRegListReserved

kDataAddrRegsBitmask
kRegListDataAddr

or

kReservedRegsBitmask
kDataAddrRegsBitmask

kRegListReserved
kRegListDataAddr


That said, using non-POD static constructors is against our coding style.  It
would be better not to introduce another non-POD usage here, and to move the
RegisterList() call into the use sites.  (Or can this be made non-POD by
inlining the constructor?  I'm not sure, but I don't think it can.)

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/nc...
File src/trusted/validator_mips/ncvalidate.cc (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/nc...
src/trusted/validator_mips/ncvalidate.cc:20: using
nacl_mips_dec::kRegListDataAddr;
Wouldn't it be better to use the fully-qualified name below rather than use
"using"?

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/nc...
src/trusted/validator_mips/ncvalidate.cc:89: 1U * (1 << NACL_MAX_ADDR_BITS),  //
bytes of data space // keep in sync w/
Hmm, as an aside, this code is duplicated between here and ncval.cc.  Maybe you
could clean this up in a separate change?

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/va...
File src/trusted/validator_mips/validator_tests.cc (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/va...
src/trusted/validator_mips/validator_tests.cc:148: * of kRegListDataAddr at the
top of this file.
Update comment?  kRegListDataAddr isn't defined at the top of the file.

[JF, 2013-01-15 17:03:03]

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/mo...
File src/trusted/validator_mips/model.h (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/mo...
src/trusted/validator_mips/model.h:112: static RegisterList const
kRegListReserved = RegisterList(kReservedRegsBitmask);
Agreed with Mark on non-POD, I made a similar change to the ARM code a while
ago.

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/nc...
File src/trusted/validator_mips/ncvalidate.cc (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/nc...
src/trusted/validator_mips/ncvalidate.cc:89: 1U * (1 << NACL_MAX_ADDR_BITS),  //
bytes of data space // keep in sync w/
On 2013/01/15 16:51:36, Mark Seaborn wrote:
> Hmm, as an aside, this code is duplicated between here and ncval.cc.  Maybe
you
> could clean this up in a separate change?

This code should stay, and ncval.cc should be deleted: Vlad created
src/trusted/validator/driver/ncval.cc which is a validator driver that builds on
all hosts and can handle any target. This should be the one and only stand-alone
validator :-)

[petarj, 2013-01-22 22:57:30]

Second patch set uploaded.

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/mo...
File src/trusted/validator_mips/model.h (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/mo...
src/trusted/validator_mips/model.h:112: static RegisterList const
kRegListReserved = RegisterList(kReservedRegsBitmask);
On 2013/01/15 16:51:36, Mark Seaborn wrote:
> Nit: Can you make the ordering of declarations consistent?
> 
> Either:
> 
> kReservedRegsBitmask
> kRegListReserved
> 
> kDataAddrRegsBitmask
> kRegListDataAddr
> 
> or
> 
> kReservedRegsBitmask
> kDataAddrRegsBitmask
> 
> kRegListReserved
> kRegListDataAddr
> 
> 
> That said, using non-POD static constructors is against our coding style.  It
> would be better not to introduce another non-POD usage here, and to move the
> RegisterList() call into the use sites.  (Or can this be made non-POD by
> inlining the constructor?  I'm not sure, but I don't think it can.)

Done.

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/mo...
src/trusted/validator_mips/model.h:112: static RegisterList const
kRegListReserved = RegisterList(kReservedRegsBitmask);
On 2013/01/15 17:03:03, JF wrote:
> Agreed with Mark on non-POD, I made a similar change to the ARM code a while
> ago.
Done (similar to the ARM change).

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/nc...
File src/trusted/validator_mips/ncvalidate.cc (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/nc...
src/trusted/validator_mips/ncvalidate.cc:89: 1U * (1 << NACL_MAX_ADDR_BITS),  //
bytes of data space // keep in sync w/
On 2013/01/15 16:51:36, Mark Seaborn wrote:
> Hmm, as an aside, this code is duplicated between here and ncval.cc.  Maybe
you
> could clean this up in a separate change?
I will clean it up in a separate change.
So we do not the local ncval.cc anymore, and will be using
driver/ncval.cc only, right?

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/va...
File src/trusted/validator_mips/validator_tests.cc (right):

https://codereview.chromium.org/11876041/diff/1/src/trusted/validator_mips/va...
src/trusted/validator_mips/validator_tests.cc:148: * of kRegListDataAddr at the
top of this file.
On 2013/01/15 16:51:36, Mark Seaborn wrote:
> Update comment?  kRegListDataAddr isn't defined at the top of the file.

Done.

[JF, 2013-01-23 00:38:02]

lgtm

> > Hmm, as an aside, this code is duplicated between here and ncval.cc.  Maybe
> you
> > could clean this up in a separate change?
> I will clean it up in a separate change.
> So we do not the local ncval.cc anymore, and will be using
> driver/ncval.cc only, right?

That is my understanding, though Vlad may be expecting that you'll contribute
the MIPS code to ncval.cc :-)

[petarj, 2013-01-23 00:40:00]

On 2013/01/23 00:38:02, JF wrote:
> That is my understanding, though Vlad may be expecting that you'll contribute
> the MIPS code to ncval.cc :-)

That's fine. :)

[commit-bot: I haz the power, 2013-01-23 03:21:45]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[petarj, 2013-01-25 08:11:21]

On 2013/01/23 03:21:45, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.

Mark?

[Mark Seaborn, 2013-01-29 16:24:48]

LGTM with fix below.

I would have much preferred if you had done the cleanup change separately from
the functional change.  The functional change is quite obscured by the cleanup. 
But I won't hold up this change any further since validator_mips is basically
yours.

https://codereview.chromium.org/11876041/diff/17001/src/trusted/validator_mip...
File src/trusted/validator_mips/ncval.cc (right):

https://codereview.chromium.org/11876041/diff/17001/src/trusted/validator_mip...
src/trusted/validator_mips/ncval.cc:62: RegisterList::DataAddrRegs());  // Data
addressing register ($sp).
Comment "$sp" is misleading now.  It should be "// Data addressing registers."

[commit-bot: I haz the power, 2013-01-29 16:49:32]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11876041/24001

[commit-bot: I haz the power, 2013-01-29 16:51:04]

Try job failure for 11876041-24001 on nacl-win8_glibc_opt for step "update".
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-win8_glib...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[petarj, 2013-01-29 17:11:34]

On 2013/01/29 16:51:04, I haz the power (commit-bot) wrote:
> Try job failure for 11876041-24001 on nacl-win8_glibc_opt for step "update".
>
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-win8_glib...
> 
> Step "update" is always a major failure.
> Look at the try server FAQ for more details.

Is CQ working? nacl-win8_glibc_opt seems to be out of order.

[petarj, 2013-01-29 18:50:53]

On 2013/01/29 17:11:34, petarj wrote:
> On 2013/01/29 16:51:04, I haz the power (commit-bot) wrote:
> > Try job failure for 11876041-24001 on nacl-win8_glibc_opt for step "update".
> >
>
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-win8_glib...
> > 
> > Step "update" is always a major failure.
> > Look at the try server FAQ for more details.
> 
> Is CQ working? nacl-win8_glibc_opt seems to be out of order.

Why is nacl-win8_glibc_opt included in the trybot list? It seems nobody is
building there except this CL.

[Mark Seaborn, 2013-01-29 20:38:38]

On 29 January 2013 10:50, <petarj@mips.com> wrote:

> On 2013/01/29 17:11:34, petarj wrote:
>
>> On 2013/01/29 16:51:04, I haz the power (commit-bot) wrote:
>> > Try job failure for 11876041-24001 on nacl-win8_glibc_opt for step
>> "update".
>>
>http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-win8_glibc_opt&number=5
>>
> >
>> > Step "update" is always a major failure.
>> > Look at the try server FAQ for more details.
>>
>
>  Is CQ working? nacl-win8_glibc_opt seems to be out of order.
>>
>
> Why is nacl-win8_glibc_opt included in the trybot list? It seems nobody is
> building there except this CL.
>

@Brad: Could you fix the Windows 8 trybots or remove them from the Buildbot
config, please?

Thanks,
Mark

-- 
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 http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[commit-bot: I haz the power, 2013-01-30 01:12:36]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11876041/24001

[commit-bot: I haz the power, 2013-01-30 12:12:48]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11876041/24001

[petarj, 2013-01-30 16:05:54]

On 2013/01/30 12:12:48, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://nativeclient-status.appspot.com/cq/petarj%40mips.com/11876041/24001

CQ is not definitely working anymore. Who we gonna call?

If there's something weird and it don't look good
Who you gonna call?
Marc-Antoine Ruel

[M-A Ruel, 2013-01-30 16:33:43]

On 2013/01/30 16:05:54, petarj wrote:
> On 2013/01/30 12:12:48, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://nativeclient-status.appspot.com/cq/petarj%2540mips.com/11876041/24001
> 
> CQ is not definitely working anymore. Who we gonna call?
> 
> If there's something weird and it don't look good
> Who you gonna call?
> Marc-Antoine Ruel

Sadly, nobody is actively working on the NaCl CQ. It's still using the old TS
interface because of bugs in Rietveld that are deemed not worth fixing, as we
just want to ditch Rietveld altogether.

[petarj, 2013-01-30 17:40:35]

On 2013/01/30 16:33:43, Marc-Antoine Ruel wrote:
> On 2013/01/30 16:05:54, petarj wrote:
> > On 2013/01/30 12:12:48, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >
>
https://nativeclient-status.appspot.com/cq/petarj%252540mips.com/11876041/24001
> > 
> > CQ is not definitely working anymore. Who we gonna call?
> > 
> > If there's something weird and it don't look good
> > Who you gonna call?
> > Marc-Antoine Ruel
> 
> Sadly, nobody is actively working on the NaCl CQ. It's still using the old TS
> interface because of bugs in Rietveld that are deemed not worth fixing, as we
> just want to ditch Rietveld altogether.

If nobody else has something to add, I will commit this with NOTRY=true.

The tries on trybots have succeeded when manually run.

[commit-bot: I haz the power, 2013-01-30 22:43:09]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11876041/24001

[commit-bot: I haz the power, 2013-01-30 23:28:45]

Change committed as 10686