Subzero: Speed up VariablesMetadata initialization.

Subzero: Speed up VariablesMetadata initialization.

Currently, O2 calls VariablesMetadata::init() 4 times:

- Twice for liveness analysis, where only multi-block use information is needed for dealing with sparse bit vectors.

- Once for address mode inference, where single-definition information is needed.

- Once for register allocation, where all information is needed, including the set of all definitions which is needed for determining AllowOverlap.

So we limit the amount of data we gather based on the actual need.

BUG= none
R=jvoung@chromium.org

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

[Jim Stichnoth, 2014-10-15 17:09:31]

stichnot@chromium.org changed reviewers:
+ jvoung@chromium.org

[Jim Stichnoth, 2014-10-15 17:10:07]

This gives 3-5% improvement in O2 translation time.

[jvoung (off chromium), 2014-10-15 17:44:55]

otherwise LGTM

https://codereview.chromium.org/650613003/diff/1/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/650613003/diff/1/src/IceOperand.cpp#newcode249
src/IceOperand.cpp:249: if (FirstDefinition == NULL)
Looks like this could be put under "if (TrackingKind == VMK_All)" as well, since
VariablesMetadata::getFirstDefinition asserts that it is VMK_All.

Maybe the assert above w.r.t. instructions in the Definitions list can also be
under the same "if () {}"?

[Jim Stichnoth, 2014-10-15 21:29:43]

PTAL.

I changed the meaning of Definitions[] to be all definitions except the first
one, so that there is no longer duplication between FirstOrSingleDefinition and
Definitions[0].  And for the many variables with a single definition, we don't
need to add the definition to the Definitions[] container.  This saves another
1-2%.

https://codereview.chromium.org/650613003/diff/1/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/650613003/diff/1/src/IceOperand.cpp#newcode249
src/IceOperand.cpp:249: if (FirstDefinition == NULL)
On 2014/10/15 17:44:54, jvoung wrote:
> Looks like this could be put under "if (TrackingKind == VMK_All)" as well,
since
> VariablesMetadata::getFirstDefinition asserts that it is VMK_All.
> 
> Maybe the assert above w.r.t. instructions in the Definitions list can also be
> under the same "if () {}"?

Renamed the field to FirstOrSingleDefinition to clarify.  Also changed the
assert in VariablesMetadata::getSingleDefinition() to be the same as
getFirstDefinition(), i.e. less restrictive.

[jvoung (off chromium), 2014-10-15 22:10:37]

LGTM

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

https://codereview.chromium.org/650613003/diff/40001/src/IceRegAlloc.cpp#newc...
src/IceRegAlloc.cpp:56: for (size_t i = 0; i < Defs.size(); ++i) {
Might be able to use a range-based for-loop now that "i > 0" isn't checked?

[Jim Stichnoth, 2014-10-15 22:12:44]

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

https://codereview.chromium.org/650613003/diff/40001/src/IceRegAlloc.cpp#newc...
src/IceRegAlloc.cpp:56: for (size_t i = 0; i < Defs.size(); ++i) {
On 2014/10/15 22:10:37, jvoung wrote:
> Might be able to use a range-based for-loop now that "i > 0" isn't checked?

Yeah, though I was thinking of making one big pass over the code to change most
or all loops involving std::vector.size() to be range-based.

[Jim Stichnoth, 2014-10-15 22:13:12]

Committed patchset #3 (id:40001) manually as
877b04e409637216712d3c36fc155b47f8bd8d38 (presubmit successful).