Subzero. Performance tweaks.

Subzero. Performance tweaks.

Introduces Ice::SmallBitVector.

Modifies ConstantRelocatables so that known offsets (i.e.,
not offsets to the code stream) do no require GlobalContext
allocations.

Modifies Cfg-local containers to use the CfgLocalAllocator.

BUG=
R=stichnot@chromium.org

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

[John, 2016-02-24 19:19:35]

Description was changed from

==========
Subzero. Performance tweaks.

Introduces Ice::SmallBitVector.

Modifies Cfg-local containers to use the CfgLocalAllocator.

BUG=
==========

to

==========
Subzero. Performance tweaks.

Introduces Ice::SmallBitVector.

Modifies ConstantRelocatables so that known offsets (i.e.,
not offsets to the code stream.)

Modifies Cfg-local containers to use the CfgLocalAllocator.

BUG=
==========

[John, 2016-02-24 19:19:35]

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

[John, 2016-02-24 19:25:21]

Description was changed from

==========
Subzero. Performance tweaks.

Introduces Ice::SmallBitVector.

Modifies ConstantRelocatables so that known offsets (i.e.,
not offsets to the code stream.)

Modifies Cfg-local containers to use the CfgLocalAllocator.

BUG=
==========

to

==========
Subzero. Performance tweaks.

Introduces Ice::SmallBitVector.

Modifies ConstantRelocatables so that known offsets (i.e.,
not offsets to the code stream) do no require GlobalContext
allocations.

Modifies Cfg-local containers to use the CfgLocalAllocator.

BUG=
==========

[Jim Stichnoth, 2016-02-24 23:11:39]

Initial round.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h
File src/IceBitVector.h (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:1: 
remove leading blank line

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:12: /// \brief Defines and implements a bit vector with
inline storage. It is a
reflow comment

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:13: /// drop in replacement for llvm::SmallSmallBitVector in
subzero -- i.e., not
SmallSmall
here and below

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:49: static_assert(sizeof(Bits) == 16, "");
Can you remove this?  Seems to be also tested below.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:55: assert(Size <= sizeof(Bits) * CHAR_BIT);
constexpr

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:76: Reference(uint64_t *D, SizeT B) : Data(D), Bit(B) {
ElementType instead of uint64_t ?

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:86: // return Reference(Bits + (Idx >>
llvm::Log2_64(NumBitsPerPos)),
remove commented code

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:88: return Reference(Bits + (Idx >> 6), Idx & ((1ull << 6) -
1));
All these "6" should be constexpr, with a static_assert to validate.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:93: return Bits[Idx >> 6] & (1ull << (Idx & ((1ull << 6) -
1)));
I think it would be nicer for the 1ull to be a constexpr.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:100: bool any() const { return Bits[0] != 0 || Bits[1] != 0;
}
Either unroll this automatically like above, or static_assert.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:124: // set_all<0>();
remove comment

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:128: return llvm::countPopulation(Bits[0]) +
llvm::countPopulation(Bits[1]);
inline/static_assert
here and below

[Jim Stichnoth, 2016-02-24 23:23:32]

More comments

https://codereview.chromium.org/1738443002/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceAssembler.h#newc...
src/IceAssembler.h:341: ArenaAllocator Allocator;
So the assembler arena size increases now, hope that's not a perf issue.  I
recall this being tweaked a long time ago for perf reasons.

https://codereview.chromium.org/1738443002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:54: // minimize the interation between the bits in
OffsetExpr.size() and
interaction

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.cpp
File src/IceMemory.cpp (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.cpp#newcode1
src/IceMemory.cpp:1: //===- subzero/src/IceMemory.h - Memory management
declarations -*- C++ -*-===//
.cpp / definitions

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.cpp#newco...
src/IceMemory.cpp:31: auto *Allocator = Manager != nullptr ?
Manager->Allocator.get() : nullptr;
I would slightly prefer:

  Manager == nullptr ? nullptr : Manager->Allocator.get();

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.h
File src/IceMemory.h (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.h#newcode40
src/IceMemory.h:40: LockedArenaAllocator(ArenaAllocator *Alloc, std::mutex
*Mutex) : Alloc(Alloc), AutoLock(*Mutex) {}
80-col

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.h#newcode52
src/IceMemory.h:52: template <typename T, typename Traits> struct sz_allocator {
sz_allocator/allocator_type/manager_type don't follow LLVM coding style?

[Jim Stichnoth, 2016-02-24 23:31:25]

https://codereview.chromium.org/1738443002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1738443002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1384: // reported, without adding extra checks to the
translator for such
reflow comment

[John, 2016-02-24 23:46:06]

https://codereview.chromium.org/1738443002/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceAssembler.h#newc...
src/IceAssembler.h:341: ArenaAllocator Allocator;
On 2016/02/24 23:23:32, stichnot wrote:
> So the assembler arena size increases now, hope that's not a perf issue.  I
> recall this being tweaked a long time ago for perf reasons.

It was very unfortunate that ArenaAllocator was a template -- we had <> spread
all over the place because of that. I defined a new, assembler-local arena type.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h
File src/IceBitVector.h (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:1: 
On 2016/02/24 23:11:39, stichnot wrote:
> remove leading blank line

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:12: /// \brief Defines and implements a bit vector with
inline storage. It is a
On 2016/02/24 23:11:38, stichnot wrote:
> reflow comment

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:13: /// drop in replacement for llvm::SmallSmallBitVector in
subzero -- i.e., not
On 2016/02/24 23:11:39, stichnot wrote:
> SmallSmall
> here and below

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:49: static_assert(sizeof(Bits) == 16, "");
On 2016/02/24 23:11:38, stichnot wrote:
> Can you remove this?  Seems to be also tested below.

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:55: assert(Size <= sizeof(Bits) * CHAR_BIT);
On 2016/02/24 23:11:39, stichnot wrote:
> constexpr

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:76: Reference(uint64_t *D, SizeT B) : Data(D), Bit(B) {
On 2016/02/24 23:11:38, stichnot wrote:
> ElementType instead of uint64_t ?

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:86: // return Reference(Bits + (Idx >>
llvm::Log2_64(NumBitsPerPos)),
On 2016/02/24 23:11:39, stichnot wrote:
> remove commented code

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:88: return Reference(Bits + (Idx >> 6), Idx & ((1ull << 6) -
1));
On 2016/02/24 23:11:39, stichnot wrote:
> All these "6" should be constexpr, with a static_assert to validate.

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:93: return Bits[Idx >> 6] & (1ull << (Idx & ((1ull << 6) -
1)));
On 2016/02/24 23:11:39, stichnot wrote:
> I think it would be nicer for the 1ull to be a constexpr.

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:100: bool any() const { return Bits[0] != 0 || Bits[1] != 0;
}
On 2016/02/24 23:11:39, stichnot wrote:
> Either unroll this automatically like above, or static_assert.

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:124: // set_all<0>();
On 2016/02/24 23:11:38, stichnot wrote:
> remove comment

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceBitVector.h#newc...
src/IceBitVector.h:128: return llvm::countPopulation(Bits[0]) +
llvm::countPopulation(Bits[1]);
On 2016/02/24 23:11:39, stichnot wrote:
> inline/static_assert
> here and below

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:54: // minimize the interation between the bits in
OffsetExpr.size() and
On 2016/02/24 23:23:32, stichnot wrote:
> interaction

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.cpp
File src/IceMemory.cpp (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.cpp#newcode1
src/IceMemory.cpp:1: //===- subzero/src/IceMemory.h - Memory management
declarations -*- C++ -*-===//
On 2016/02/24 23:23:32, stichnot wrote:
> .cpp / definitions

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.cpp#newco...
src/IceMemory.cpp:31: auto *Allocator = Manager != nullptr ?
Manager->Allocator.get() : nullptr;
On 2016/02/24 23:23:32, stichnot wrote:
> I would slightly prefer:
> 
>   Manager == nullptr ? nullptr : Manager->Allocator.get();

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.h
File src/IceMemory.h (right):

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.h#newcode40
src/IceMemory.h:40: LockedArenaAllocator(ArenaAllocator *Alloc, std::mutex
*Mutex) : Alloc(Alloc), AutoLock(*Mutex) {}
On 2016/02/24 23:23:32, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1738443002/diff/20001/src/IceMemory.h#newcode52
src/IceMemory.h:52: template <typename T, typename Traits> struct sz_allocator {
On 2016/02/24 23:23:32, stichnot wrote:
> sz_allocator/allocator_type/manager_type don't follow LLVM coding style?

they follow c++'s naming convention.

https://codereview.chromium.org/1738443002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1738443002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1384: // reported, without adding extra checks to the
translator for such
On 2016/02/24 23:31:24, stichnot wrote:
> reflow comment

Done.

[Jim Stichnoth, 2016-02-24 23:50:47]

lgtm

[John, 2016-02-24 23:58:58]

Description was changed from

==========
Subzero. Performance tweaks.

Introduces Ice::SmallBitVector.

Modifies ConstantRelocatables so that known offsets (i.e.,
not offsets to the code stream) do no require GlobalContext
allocations.

Modifies Cfg-local containers to use the CfgLocalAllocator.

BUG=
==========

to

==========
Subzero. Performance tweaks.

Introduces Ice::SmallBitVector.

Modifies ConstantRelocatables so that known offsets (i.e.,
not offsets to the code stream) do no require GlobalContext
allocations.

Modifies Cfg-local containers to use the CfgLocalAllocator.

BUG=
R=stichnot@chromium.org

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

[John, 2016-02-24 23:58:59]

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