Subzero. ARM32. Materializes the register table.

Subzero. ARM32. Materializes the register table.

This CL modifies the ARM32 backend so that the REGARM32_TABLE is only
expanded once (to initialize a constexpr array.) This change decreased
the backend size in roughly ~80k.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=kschimpf@google.com

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

[John, 2016-01-04 19:52:57]

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

[Karl, 2016-01-04 21:00:47]

lgtm

[Karl, 2016-01-04 21:10:45]

https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:118: static constexpr struct TableType {
Why is this a constexpr? Won't functions like getEncodedGPR and getEncodedSReg
need to store DATA copies to work?

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

https://codereview.chromium.org/1554263002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:60: constexpr RegARM32::TableType
RegARM32::Table[];
Does this generate another copy for use below?

[John, 2016-01-04 21:45:12]

https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:118: static constexpr struct TableType {
On 2016/01/04 21:10:45, Karl wrote:
> Why is this a constexpr? Won't functions like getEncodedGPR and getEncodedSReg
> need to store DATA copies to work?

resolved offline.

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

https://codereview.chromium.org/1554263002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:60: constexpr RegARM32::TableType
RegARM32::Table[];
On 2016/01/04 21:10:45, Karl wrote:
> Does this generate another copy for use below?

This is the definition for the array declaration. Without it, we get linker
errors.

[John, 2016-01-04 21:45:29]

Description was changed from

==========
Subzero. ARM32. Materializes the register table.

This CL modifies the ARM32 backend so that the REGARM32_TABLE is only
expanded once (to initialize a constexpr array.) This change decreased
the backend size in roughly ~80k.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Materializes the register table.

This CL modifies the ARM32 backend so that the REGARM32_TABLE is only
expanded once (to initialize a constexpr array.) This change decreased
the backend size in roughly ~80k.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=kschimpf@google.com

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

[John, 2016-01-04 21:45:30]

Committed patchset #2 (id:20001) manually as
149999e54156c3ee219cee9a2c63be35437ae341 (presubmit successful).

[Jim Stichnoth, 2016-01-06 19:41:41]

https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:41: template <typename T, typename... U> struct __length
{
I believe that identifiers containing adjacent underscores are not allowed
(reserved for the language implementation).  I take it to mean it's perhaps OK
for the szrt, but not here.

In fact, it looks like we're already violating that in a number of places...

[native-client-reviews_googlegroups.com, 2016-01-06 20:44:53]

Only for globals:
http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-u...

I guess the rationale is that a global __length() method is more likely to
clash than an class method deep down in a namespace/struct.

Plus, this should go away once we move to c++14.

On Wed, Jan 6, 2016 at 11:41 AM, <stichnot@chromium.org> wrote:

>
>
> https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h
> File src/IceRegistersARM32.h (right):
>
>
>
https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h...
> src/IceRegistersARM32.h:41: template <typename T, typename... U> struct
> __length {
> I believe that identifiers containing adjacent underscores are not
> allowed (reserved for the language implementation).  I take it to mean
> it's perhaps OK for the szrt, but not here.
>
> In fact, it looks like we're already violating that in a number of
> places...
>
> https://codereview.chromium.org/1554263002/
>

-- 
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.

[native-client-reviews_googlegroups.com, 2016-01-06 20:47:38]

well, not quite:

http://en.cppreference.com/w/cpp/language/identifiers

"the identifiers with a double underscore anywhere are reserved;"

On Wed, Jan 6, 2016 at 12:44 PM, João Porto <jpp@google.com> wrote:

> Only for globals:
>
http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-u...
>
> I guess the rationale is that a global __length() method is more likely to
> clash than an class method deep down in a namespace/struct.
>
> Plus, this should go away once we move to c++14.
>
> On Wed, Jan 6, 2016 at 11:41 AM, <stichnot@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h
>> File src/IceRegistersARM32.h (right):
>>
>>
>>
https://codereview.chromium.org/1554263002/diff/20001/src/IceRegistersARM32.h...
>> src/IceRegistersARM32.h:41: template <typename T, typename... U> struct
>> __length {
>> I believe that identifiers containing adjacent underscores are not
>> allowed (reserved for the language implementation).  I take it to mean
>> it's perhaps OK for the szrt, but not here.
>>
>> In fact, it looks like we're already violating that in a number of
>> places...
>>
>> https://codereview.chromium.org/1554263002/
>>
>
>

-- 
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.