Subzero: Improve usability of liveness-related tools.

Subzero: Improve usability of liveness-related tools.

1. Rename all identifiers containing "nonkillable" to use the more understandable "redefined".

2. Change inferTwoAddress() to be called inferRedefinition(), and to check *all* instruction source variables (instead of just the first source operand) against the Dest variable.  This eliminates the need for several instances of _set_dest_redefined().  The performance impact on translation time is something like 0.1%, which is dwarfed by the usability gain.

3. Change a cryptic assert in (O2) live range construction to print detailed information on the liveness errors.

4. Change a cryptic assert in (Om1) live range construction to do the same.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2015-09-25 20:34:51]

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

[John, 2015-09-25 23:02:17]

lgtm

just suggestions. lgtm otherwise.

By the way, I just thought that maybe "_redefining_mov" is a more accurate name
(it sounds more accurate to me, so it probably isn't :))

https://codereview.chromium.org/1368993004/diff/20001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1368993004/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:701: if (BuildDefs::asserts()) {
this would be easier to read (I think) if written as

if (!asserts())
  return;

failure = std::adjacent_find() != ..

if (!dump()) {
  assert(!failure);
  return;
}

if (!failure)
  return;

for (...)

It is easier to read if you don't have several nesting levels.

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:171: llvm::SmallVector<SizeT, 10> DefWithoutUse,
UseBeforeDef;
Maybe plural? I.e., DefsWithoutUses

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:220: if (BuildDefs::dump()) {
same thing here

if (DefWithoutUse.empty() && UseBeforeDef.empty())
  return;

if (!dump)
  llvm_report_fatal();

for (...)

for (...)

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:223: for (SizeT i : DefWithoutUse) {
'i' is weird here. I mostly see single-letter variables when you have an index.
You arguably have an index here, but I would rather it to be named Def

https://codereview.chromium.org/1368993004/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1368993004/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:293: void inferRedefinition();
this does not infer, it finds. maybe rename it findRedefinitions? or, better
yet, markRedefinitions?

[Jim Stichnoth, 2015-09-25 23:56:01]

Thanks!

https://codereview.chromium.org/1368993004/diff/20001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1368993004/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:701: if (BuildDefs::asserts()) {
On 2015/09/25 23:02:16, John wrote:
> this would be easier to read (I think) if written as
> 
> if (!asserts())
>   return;
> 
> failure = std::adjacent_find() != ..
> 
> if (!dump()) {
>   assert(!failure);
>   return;
> }
> 
> if (!failure)
>   return;
> 
> for (...)
> 
> It is easier to read if you don't have several nesting levels.

Done.

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:171: llvm::SmallVector<SizeT, 10> DefWithoutUse,
UseBeforeDef;
On 2015/09/25 23:02:16, John wrote:
> Maybe plural? I.e., DefsWithoutUses

Done.

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:220: if (BuildDefs::dump()) {
On 2015/09/25 23:02:16, John wrote:
> same thing here
> 
> if (DefWithoutUse.empty() && UseBeforeDef.empty())
>   return;
> 
> if (!dump)
>   llvm_report_fatal();
> 
> for (...)
> 
> for (...)

Done.

https://codereview.chromium.org/1368993004/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:223: for (SizeT i : DefWithoutUse) {
On 2015/09/25 23:02:16, John wrote:
> 'i' is weird here. I mostly see single-letter variables when you have an
index.
> You arguably have an index here, but I would rather it to be named Def

Done.

https://codereview.chromium.org/1368993004/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1368993004/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:293: void inferRedefinition();
On 2015/09/25 23:02:16, John wrote:
> this does not infer, it finds. maybe rename it findRedefinitions? or, better
> yet, markRedefinitions?

Done.

[Jim Stichnoth, 2015-09-26 00:40:39]

Committed patchset #4 (id:60001) manually as
230d4101fb3c591d044406eef27d0ce43ffab53e (tree was closed).