Subzero: Emit functions and global initializers in a separate thread.

Subzero: Emit functions and global initializers in a separate thread.

(This is a continuation of https://codereview.chromium.org/876083007/ .)

Emission is done in a separate thread when -threads=N with N>0 is specified.  This includes both functions and global initializers.

Emission is deterministic.  The parser assigns sequence numbers, and the emitter thread reassembles work units into their original order, regardless of the number of threads.

Dump output, however, is not intended to be in deterministic, reassembled order.  As such, lit tests that test dump output (i.e., '-verbose inst') are explicitly run with -threads=0.

For -elf-writer and -ias=1, the translator thread invokes Cfg::emitIAS() and the assembler buffer is passed to the emitter thread.  For -ias=0, the translator thread passed the Cfg to the emitter thread which then invokes Cfg::emit() to produce the textual asm.

Minor cleanup along the way:
  * Removed Flags from the Ice::Translator object and ctor, since it was redundant with Ctx->getFlags().
  * Cfg::getAssembler<> is the same as Cfg::getAssembler<Assembler> and is useful for just passing the assembler around.
  * Removed the redundant Ctx argument from TargetDataLowering::lowerConstants() .

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4075
R=jvoung@chromium.org

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

[Jim Stichnoth, 2015-02-11 05:42:43]

stichnot@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org, kschimpf@google.com

[Jim Stichnoth, 2015-02-11 05:45:41]

This new CL is a continuation of the previous one after addressing all comments
and rebasing.

[jvoung (off chromium), 2015-02-11 16:56:01]

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:185: Cfg::setCurrentCfg(nullptr);
Could add a "continue;" here, and not have to indent the following "else { }".
Also makes clear that setCurrentCfg(nullptr) is the last thing in the loop?

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:235: *VariableDeclarations.get()) {
Does unique_ptr have an "operator*", so you wouldn't need *p.get() ?

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:239:
DataLowering->lowerGlobalsELF(*VariableDeclarations.get()); // XXX
XXX ?

Also, same question about *p.get() here and below.

https://codereview.chromium.org/916653004/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/916653004/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:16: // lowering/emission of global initializers.
and constant pools

[Karl, 2015-02-11 17:52:20]

Unable to find any other issues than brought up by Jan.

[Jim Stichnoth, 2015-02-11 21:52:58]

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:185: Cfg::setCurrentCfg(nullptr);
On 2015/02/11 16:56:01, jvoung wrote:
> Could add a "continue;" here, and not have to indent the following "else { }".
> Also makes clear that setCurrentCfg(nullptr) is the last thing in the loop?

Done.

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:235: *VariableDeclarations.get()) {
On 2015/02/11 16:56:00, jvoung wrote:
> Does unique_ptr have an "operator*", so you wouldn't need *p.get() ?

It does, and this would change it to **VariableDeclarations.

I'm new to unique_ptrs, but I feel like it's better to use .get() explicitly,
since along with .release(), it makes it a bit easier to track places where raw
pointers might be escaping and potentially used after deleting.

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:239:
DataLowering->lowerGlobalsELF(*VariableDeclarations.get()); // XXX
On 2015/02/11 16:56:01, jvoung wrote:
> XXX ?
> 
> Also, same question about *p.get() here and below.

Oops, I meant to clean that up, because it seemed like lowerGlobalsELF() should
take a unique_ptr.

The latest patch set goes ahead and unifies lowerGlobalsELF() and
lowerGlobals().  The only uses of VariableDeclarationList remaining without
unique_ptr are in the ELF writer and the emit queue.

https://codereview.chromium.org/916653004/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/916653004/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:16: // lowering/emission of global initializers.
On 2015/02/11 16:56:01, jvoung wrote:
> and constant pools

Done.

[Karl, 2015-02-11 21:58:19]

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:235: *VariableDeclarations.get()) {
On 2015/02/11 21:52:58, stichnot wrote:
> On 2015/02/11 16:56:00, jvoung wrote:
> > Does unique_ptr have an "operator*", so you wouldn't need *p.get() ?
> 
> It does, and this would change it to **VariableDeclarations.
> 
> I'm new to unique_ptrs, but I feel like it's better to use .get() explicitly,
> since along with .release(), it makes it a bit easier to track places where
raw
> pointers might be escaping and potentially used after deleting.

Actually no. It would change to *VariableDeclarations. The definition of
operator* is *VariableDeclarations.get().

[Karl, 2015-02-11 21:58:20]

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:235: *VariableDeclarations.get()) {
On 2015/02/11 21:52:58, stichnot wrote:
> On 2015/02/11 16:56:00, jvoung wrote:
> > Does unique_ptr have an "operator*", so you wouldn't need *p.get() ?
> 
> It does, and this would change it to **VariableDeclarations.
> 
> I'm new to unique_ptrs, but I feel like it's better to use .get() explicitly,
> since along with .release(), it makes it a bit easier to track places where
raw
> pointers might be escaping and potentially used after deleting.

Actually no. It would change to *VariableDeclarations. The definition of
operator* is *VariableDeclarations.get().

[Jim Stichnoth, 2015-02-11 23:01:07]

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/916653004/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:235: *VariableDeclarations.get()) {
On 2015/02/11 21:58:20, Karl wrote:
> On 2015/02/11 21:52:58, stichnot wrote:
> > On 2015/02/11 16:56:00, jvoung wrote:
> > > Does unique_ptr have an "operator*", so you wouldn't need *p.get() ?
> > 
> > It does, and this would change it to **VariableDeclarations.
> > 
> > I'm new to unique_ptrs, but I feel like it's better to use .get()
explicitly,
> > since along with .release(), it makes it a bit easier to track places where
> raw
> > pointers might be escaping and potentially used after deleting.
> 
> Actually no. It would change to *VariableDeclarations. The definition of
> operator* is *VariableDeclarations.get().

Oops, you're right.

I made that change (in 3 places) where the unique_ptr is used in a range-based
for loop and therefore has no chance of escaping.

[Karl, 2015-02-11 23:03:04]

Looks good to me.

[jvoung (off chromium), 2015-02-11 23:24:15]

LGTM

https://codereview.chromium.org/916653004/diff/120001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/916653004/diff/120001/src/IceCfg.cpp#newcode425
src/IceCfg.cpp:425: Assembler *Asm) {
Asm could probably be a const ptr, since it's just checking a flag?

[Jim Stichnoth, 2015-02-12 00:08:15]

New patchsets have been uploaded after l-g-t-m from jvoung@chromium.org

[Jim Stichnoth, 2015-02-12 00:08:26]

https://codereview.chromium.org/916653004/diff/120001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/916653004/diff/120001/src/IceCfg.cpp#newcode425
src/IceCfg.cpp:425: Assembler *Asm) {
On 2015/02/11 23:24:15, jvoung wrote:
> Asm could probably be a const ptr, since it's just checking a flag?

Done.

[Jim Stichnoth, 2015-02-12 00:08:35]

Committed patchset #8 (id:140001) manually as
bbca754a63fb1e80b4d0f034aa26538f3a8a2a26 (presubmit successful).