Subzero: Refactor some common TargetLowering initializations.

Subzero: Refactor some common TargetLowering initializations.

Each TargetLowering subclass has several fields (generally register allocation related) that are initialized to the same values every time a TargetLowering object is created.  These fields are essentially const once initialized, so there is no reason to repeatedly initialize them.

The solution is to make them static fields, and statically initialize them at program startup.

This also makes it practical to access such fields without needing a TargetLowering object.

There are likely more items that should also get this treatment, but those can be changed later.

The staticInit() method needs a run-once guard because the unit tests actually cause it to be called more than once.

BUG= none
R=kschimpf@google.com

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

[Jim Stichnoth, 2015-11-02 22:54:04]

Description was changed from

==========
Subzero: Refactor some common TargetLowering initializations.

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

to

==========
Subzero: Refactor some common TargetLowering initializations.

Each TargetLowering subclass has several fields (generally register allocation
related) that are initialized to the same values every time a TargetLowering
object is created.  These fields are essentially const once initialized, so
there is no reason to repeatedly initialize them.

The solution is to make them static fields, and statically initialize them at
program startup.

The staticInit() method needs a run-once guard because the unit tests actually
cause it to be called more than once.

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

[Jim Stichnoth, 2015-11-02 23:04:15]

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

[Jim Stichnoth, 2015-11-02 23:09:34]

Description was changed from

==========
Subzero: Refactor some common TargetLowering initializations.

Each TargetLowering subclass has several fields (generally register allocation
related) that are initialized to the same values every time a TargetLowering
object is created.  These fields are essentially const once initialized, so
there is no reason to repeatedly initialize them.

The solution is to make them static fields, and statically initialize them at
program startup.

The staticInit() method needs a run-once guard because the unit tests actually
cause it to be called more than once.

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

to

==========
Subzero: Refactor some common TargetLowering initializations.

Each TargetLowering subclass has several fields (generally register allocation
related) that are initialized to the same values every time a TargetLowering
object is created.  These fields are essentially const once initialized, so
there is no reason to repeatedly initialize them.

The solution is to make them static fields, and statically initialize them at
program startup.

This also makes it practical to access such fields without needing a
TargetLowering object.

There are likely more items that should also get this treatment, but those can
be changed later.

The staticInit() method needs a run-once guard because the unit tests actually
cause it to be called more than once.

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

[Jim Stichnoth, 2015-11-04 14:51:08]

Ping.  The rest of the x86 register refactoring builds on this, so I'd like to
get feedback before getting too far along on the next step.

[Karl, 2015-11-04 16:26:58]

lgtm

[John, 2015-11-04 21:35:23]

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:109: static bool InitGuard = false;
Not that this matter, but if in the future we allow multiple targets to be
loaded at the same time, this will prevent all but the first target to
initialize.

why not invoking staticInit() on everything, and letting SZTargets.def handle
things a bit?

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:91: TargetX86Base<TargetX8664>::TypeToRegisterSet
= {};
Do you really need the {}?

come to think of it, this is a non-POD type. We should define non-pod static
members.

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:61: static void staticInit();
optional: You may want to move this to the actual target (i.e., TargetX8632)
instead. If you do, you can remove the Traits function that initializes the
register set.

[Jim Stichnoth, 2015-11-04 23:51:39]

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:109: static bool InitGuard = false;
On 2015/11/04 21:35:23, John wrote:
> Not that this matter, but if in the future we allow multiple targets to be
> loaded at the same time, this will prevent all but the first target to
> initialize.
> 
> why not invoking staticInit() on everything, and letting SZTargets.def handle
> things a bit?

I did something like that originally, by duplicating the init guard stuff across
all the target source files, decided I didn't like the duplication, and
refactored it here.

But you're right, that does unnecessarily constrain the number of active targets
during a run.

Done.

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:91: TargetX86Base<TargetX8664>::TypeToRegisterSet
= {};
On 2015/11/04 21:35:23, John wrote:
> Do you really need the {}?
> 
> come to think of it, this is a non-POD type. We should define non-pod static
> members.

Without the explicit initializations (here and the two below), it fails to link
due to undefined references.  Mind blown!

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1418853005/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:61: static void staticInit();
On 2015/11/04 21:35:23, John wrote:
> optional: You may want to move this to the actual target (i.e., TargetX8632)
> instead. If you do, you can remove the Traits function that initializes the
> register set.

Acknowledged.

[Jim Stichnoth, 2015-11-05 00:06:22]

Committed patchset #3 (id:40001) manually as
94844f1290d7fae68ad4f9e9b9e3543639e44a15 (presubmit successful).