Subzero: Reduce copying of Liveness bitvectors.

Subzero: Reduce copying of Liveness bitvectors.

There were a lot of unnecessary copying and resizing and sloppiness in the use of BitVector for liveness analysis.  This tries to reduce the amount of copying and associated memory allocation.

Also, works around a ConstantRelocatable hashing problem that was causing a huge slowdown in MINIMAL mode for the vector_shuffle scons test.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-02-28 20:11:19]

Description was changed from

==========
Subzero: Reduce copying of Liveness bitvectors.

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

to

==========
Subzero: Reduce copying of Liveness bitvectors.

There were a lot of unnecessary copying and resizing and sloppiness in the use
of BitVector for liveness analysis.  This tries to reduce the amount of copying
and associated memory allocation.

Also, works around a ConstantRelocatable hashing problem that was causing a huge
slowdown  in MINIMAL mode for the vector_shuffle scons test.

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

[Jim Stichnoth, 2016-02-28 20:12:30]

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

[John, 2016-02-29 15:12:31]

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.cpp#newcode343
src/IceCfg.cpp:343: (*I)->liveness(getLiveness(), &ScratchBV);
make scratchbv a member of Liveness?

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.cpp#newcode1083
src/IceCfg.cpp:1083: return
(CfgLocalAllocator<int>().current()->getTotalMemory() / OneMB);
Add a comment explaining why int and not something else.

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.h#newcode222
src/IceCfg.h:222: static size_t getTotalMemoryMB();
why is this a member of CFG? this seems like a global function to me.

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

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:627: // the node's predecessors need to be processed again.) 
The ScratchBV is
Really, ScratchBV should not be part of the interface -- it is an implementation
detail. Please add it as member.

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:633: LivenessBV &Live = *ScratchBV;
Why are you incurring the extra memory de-reference? Just replace uses of Live
with ScratchBV?

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:654: const LivenessBV &LiveIn = Liveness->getLiveIn(Succ);
auto?

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:1365: if (Func->isVerbose(IceV_Liveness) && Liveness !=
nullptr &&
this test is testing different things. it is (to me, at least) clearer to read

if (Func->isVerbose()) {
  if (Liveness && !LiveInt.empty()) {
    dumpLiveness(LiveIn);
  }
}

[Jim Stichnoth, 2016-02-29 22:55:16]

Description was changed from

==========
Subzero: Reduce copying of Liveness bitvectors.

There were a lot of unnecessary copying and resizing and sloppiness in the use
of BitVector for liveness analysis.  This tries to reduce the amount of copying
and associated memory allocation.

Also, works around a ConstantRelocatable hashing problem that was causing a huge
slowdown  in MINIMAL mode for the vector_shuffle scons test.

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

to

==========
Subzero: Reduce copying of Liveness bitvectors.

There were a lot of unnecessary copying and resizing and sloppiness in the use
of BitVector for liveness analysis.  This tries to reduce the amount of copying
and associated memory allocation.

Also, works around a ConstantRelocatable hashing problem that was causing a huge
slowdown in MINIMAL mode for the vector_shuffle scons test.

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

[Jim Stichnoth, 2016-02-29 22:58:26]

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.cpp#newcode343
src/IceCfg.cpp:343: (*I)->liveness(getLiveness(), &ScratchBV);
On 2016/02/29 15:12:31, John wrote:
> make scratchbv a member of Liveness?

Done.

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.cpp#newcode1083
src/IceCfg.cpp:1083: return
(CfgLocalAllocator<int>().current()->getTotalMemory() / OneMB);
On 2016/02/29 15:12:30, John wrote:
> Add a comment explaining why int and not something else.

Done.

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfg.h#newcode222
src/IceCfg.h:222: static size_t getTotalMemoryMB();
On 2016/02/29 15:12:31, John wrote:
> why is this a member of CFG? this seems like a global function to me.

Added a comment that this was really for use within the debugger.

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

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:627: // the node's predecessors need to be processed again.) 
The ScratchBV is
On 2016/02/29 15:12:31, John wrote:
> Really, ScratchBV should not be part of the interface -- it is an
implementation
> detail. Please add it as member.

Done.

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:633: LivenessBV &Live = *ScratchBV;
On 2016/02/29 15:12:31, John wrote:
> Why are you incurring the extra memory de-reference? Just replace uses of Live
> with ScratchBV?

I don't think that's an actual dereference happening, right?  In the end,
Live.foo and ScratchBV->foo should have the same access cost.

In any case, this line is changed as a result of moving ScratchBV into Liveness,
so ptal.

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:654: const LivenessBV &LiveIn = Liveness->getLiveIn(Succ);
On 2016/02/29 15:12:31, John wrote:
> auto?

I'd kinda rather not in this case.

I'd be happier to do auto if getLiveIn() had a clearer name, like getLiveInBV(),
same for getLiveOutBV().  At the same time, Liveness also has getLiveBegin() and
getLiveEnd() which return a LiveBeginEndMap, and so I would also want to change
them to something like getLiveBeginMap() and getLiveEndMap() and auto-ify them
as well.

So I'd prefer to keep auto-consistency here, and do renaming and auto-ification
in a separate CL.

https://codereview.chromium.org/1746613002/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:1365: if (Func->isVerbose(IceV_Liveness) && Liveness !=
nullptr &&
On 2016/02/29 15:12:31, John wrote:
> this test is testing different things. it is (to me, at least) clearer to read
> 
> if (Func->isVerbose()) {
>   if (Liveness && !LiveInt.empty()) {
>     dumpLiveness(LiveIn);
>   }
> }

Done.

[John, 2016-03-01 00:04:13]

lgtm

[Jim Stichnoth, 2016-03-01 00:58:17]

Description was changed from

==========
Subzero: Reduce copying of Liveness bitvectors.

There were a lot of unnecessary copying and resizing and sloppiness in the use
of BitVector for liveness analysis.  This tries to reduce the amount of copying
and associated memory allocation.

Also, works around a ConstantRelocatable hashing problem that was causing a huge
slowdown in MINIMAL mode for the vector_shuffle scons test.

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

to

==========
Subzero: Reduce copying of Liveness bitvectors.

There were a lot of unnecessary copying and resizing and sloppiness in the use
of BitVector for liveness analysis.  This tries to reduce the amount of copying
and associated memory allocation.

Also, works around a ConstantRelocatable hashing problem that was causing a huge
slowdown in MINIMAL mode for the vector_shuffle scons test.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-03-01 00:58:18]

Committed patchset #4 (id:60001) manually as
1bdb73522937503501201ac387bc1477ea5fa29a (presubmit successful).