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

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

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

[Jim Stichnoth, 2015-02-07 14:52:50]

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

[Jim Stichnoth, 2015-02-07 14:53:08]

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

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:220: void lowerGlobals(GlobalContext *Ctx,
This is moved largely unchanged from Translator::lowerGlobals() in
IceTranslator.cpp.

[JF, 2015-02-08 00:29:47]

https://codereview.chromium.org/876083007/diff/20001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/876083007/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:888: VariableDeclarationList *VariableDeclarations = new
VariableDeclarationList;
unique_ptr?

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

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:139: EmitQ(/*MaxSize=*/Flags.NumTranslationThreads,
Translation an emit thread numbers should be independent.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:424: BoundedProducerConsumerQueue<Cfg> CfgQ;
Maybe this should now be the OptimizationQ?

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:442: std::vector<std::thread> EmitterThreads;
These could just be SmallVector or even std::array, since you won't spin up more
than ~16 threads. Reduces fragmentation since you'll have less allocations!

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:507: class EmitterWorkItem {
This is getting to be a pretty big file, it's probably worth moving this to its
own file.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:509: EmitterWorkItem &operator=(const EmitterWorkItem &)
= delete;
EmitterWorkItem() = delete;

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:518: WI_Cfg          // A Cfg that needs to be emitted as
"readable" assembly.
I'm not sure I get the different between asm and cfg.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:550: ~EmitterWorkItem();
Define inline, since this should do anything.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:557: Cfg *const RawFunc;
3 x unique_ptr?

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

https://codereview.chromium.org/876083007/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:407: Ice::VariableDeclarationList *VariableDeclarations;
Why this change?

Also, unique_ptr.

[Jim Stichnoth, 2015-02-08 17:11:23]

I haven't yet addressed to unique_ptr related comments.

The issue is that for emitting global declarations and initializers, the logical
work unit for the parser to send to the emitter is the
"Ice::VariableDeclarationList *VariableDeclarations" object.  However, it turns
out that the parser continues to use this object even after the call to lower
the globals.  So I need to work with Karl on the best way to safely hand off the
work, then do as as a unique_ptr.

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

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:139: EmitQ(/*MaxSize=*/Flags.NumTranslationThreads,
On 2015/02/08 00:29:47, JF wrote:
> Translation an emit thread numbers should be independent.

Oops, cargo-cult error.

The Sequential logic is the same for both queues.  I'll change it to
Flags.isSequential() after Karl's Flags refactoring lands.

Changed the emit queue to allow unlimited size, up to the hard limit on the
underlying array size.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:424: BoundedProducerConsumerQueue<Cfg> CfgQ;
On 2015/02/08 00:29:47, JF wrote:
> Maybe this should now be the OptimizationQ?

Done.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:442: std::vector<std::thread> EmitterThreads;
On 2015/02/08 00:29:47, JF wrote:
> These could just be SmallVector or even std::array, since you won't spin up
more
> than ~16 threads. Reduces fragmentation since you'll have less allocations!

16 threads?  I've forgotten how to count that low.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:507: class EmitterWorkItem {
On 2015/02/08 00:29:47, JF wrote:
> This is getting to be a pretty big file, it's probably worth moving this to
its
> own file.

Good idea, moved this into the new IceThreading.h, along with the blocking queue
stuff from IceUtils.h.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:509: EmitterWorkItem &operator=(const EmitterWorkItem &)
= delete;
On 2015/02/08 00:29:47, JF wrote:
> EmitterWorkItem() = delete;

Done.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:518: WI_Cfg          // A Cfg that needs to be emitted as
"readable" assembly.
On 2015/02/08 00:29:47, JF wrote:
> I'm not sure I get the different between asm and cfg.

Added more comments that hopefully clarify.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:550: ~EmitterWorkItem();
On 2015/02/08 00:29:47, JF wrote:
> Define inline, since this should do anything.

Tried that originally, but it's getting into icky territory (which I'm guessing
unique_ptr won't solve):

make: *** [build/Release+Asserts/assembler_ia32.o] Error 1
In file included from src/IceCfgNode.cpp:16:
In file included from src/IceCfg.h:21:
src/IceGlobalContext.h:553:3: error: deleting pointer to incomplete type
'Ice::Cfg' may cause undefined behavior [-Werror,-Wdelete-incomplete]
  delete RawFunc;
  ^      ~~~~~~~
src/IceDefs.h:49:7: note: forward declaration of 'Ice::Cfg'
class Cfg;
      ^
1 error generated.

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

https://codereview.chromium.org/876083007/diff/40001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/876083007/diff/40001/src/IceCfg.h#newcode32
src/IceCfg.h:32: 
Why not SequenceNumber?

https://codereview.chromium.org/876083007/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/876083007/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:221: VariableDeclarationList *VariableDeclarations,
Add a TODO here to change this to unique_ptr once VariableDeclarations is
parallelized (you can use my name, since I will fix it).

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/arith-opt.ll (right):

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/arith-opt.ll:6: ; RUN: %p2i -i %s --args --verbose inst
-threads=0 -ias=0 | FileCheck %s
Is -threades=0 necessary (see run-llvm2ice.py).

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/branch-simple.ll (right):

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/branch-simple.ll:9: ; RUN: %p2i -i %s --args -O2
--verbose inst -threads=0 | FileCheck %s
Same here.

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/branch-simple.ll:10: ; RUN: %p2i -i %s --args -Om1
--verbose inst -threads=0 | FileCheck %s
And here (remove -threads=0)?

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/globalrelocs.ll (right):

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/globalrelocs.ll:6: ; RUN: %lc2i -i %s --args -verbose
inst -threads=0 | %iflc FileCheck %s
Again, remove -threads=0?

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/globalrelocs.ll:9: ; RUN: %p2i -i %s --args -verbose
inst -threads=0 | FileCheck %s
Same here.

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/load.ll (right):

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/load.ll:5: ; RUN: %p2i -i %s --args --verbose inst
-threads=0 | FileCheck %s
Same here.

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/store.ll (right):

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/store.ll:5: ; RUN: %p2i -i %s --args --verbose inst
-threads=0 | FileCheck %s
Same here.

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/struct-arith.pnacl.ll (right):

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/struct-arith.pnacl.ll:6: ; RUN: %lc2i -i %s --args
--verbose inst -threads=0 | FileCheck %s
Same here.

[JF, 2015-02-08 21:15:05]

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:518: WI_Cfg          // A Cfg that needs to be emitted as
"readable" assembly.
On 2015/02/08 17:11:23, stichnot wrote:
> On 2015/02/08 00:29:47, JF wrote:
> > I'm not sure I get the different between asm and cfg.
> 
> Added more comments that hopefully clarify.

Yeah, though I'm wary of having a debugging feature that always ships with
subzero. Does WI_Cfg and related functions become a nop in release mode?

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:550: ~EmitterWorkItem();
On 2015/02/08 17:11:23, stichnot wrote:
> On 2015/02/08 00:29:47, JF wrote:
> > Define inline, since this should do anything.
> 
> Tried that originally, but it's getting into icky territory (which I'm
guessing
> unique_ptr won't solve):
> 
> make: *** [build/Release+Asserts/assembler_ia32.o] Error 1
> In file included from src/IceCfgNode.cpp:16:
> In file included from src/IceCfg.h:21:
> src/IceGlobalContext.h:553:3: error: deleting pointer to incomplete type
> 'Ice::Cfg' may cause undefined behavior [-Werror,-Wdelete-incomplete]
>   delete RawFunc;
>   ^      ~~~~~~~
> src/IceDefs.h:49:7: note: forward declaration of 'Ice::Cfg'
> class Cfg;
>       ^
> 1 error generated.

Oh yeah, include order would do that, and unique_ptr would have the same issue.

https://codereview.chromium.org/876083007/diff/40001/src/IceThreading.h
File src/IceThreading.h (right):

https://codereview.chromium.org/876083007/diff/40001/src/IceThreading.h#newco...
src/IceThreading.h:61: : std::min(MaxSize, MaxStaticSize)),
Why have UnlimitedSize at all when MaxStaticSize would do the same?

https://codereview.chromium.org/876083007/diff/40001/src/IceThreading.h#newco...
src/IceThreading.h:154: class EmitterWorkItem {
Overall this class looks like it wants to be a virtual class (with all the
different ctors and all) but is instead the union of all the derived class'
functionality. IIUC you want it all inline without abstraction to ensure that it
can be held in datastructures without indirection and extra allocations?

That's fine with me, I just want to make sure that's intentional, and if so you
should document the design choice on the class.

[Jim Stichnoth, 2015-02-10 07:51:47]

https://codereview.chromium.org/876083007/diff/20001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/876083007/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:888: VariableDeclarationList *VariableDeclarations = new
VariableDeclarationList;
On 2015/02/08 00:29:47, JF wrote:
> unique_ptr?

Yeah, after Karl's CL lands...

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:518: WI_Cfg          // A Cfg that needs to be emitted as
"readable" assembly.
On 2015/02/08 21:15:04, JF wrote:
> On 2015/02/08 17:11:23, stichnot wrote:
> > On 2015/02/08 00:29:47, JF wrote:
> > > I'm not sure I get the different between asm and cfg.
> > 
> > Added more comments that hopefully clarify.
> 
> Yeah, though I'm wary of having a debugging feature that always ships with
> subzero. Does WI_Cfg and related functions become a nop in release mode?

I added a report_fatal_error() call to GlobalContext::emitItems() in case a
WI_Cfg item somehow makes it onto the queue in a release build.

https://codereview.chromium.org/876083007/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:557: Cfg *const RawFunc;
On 2015/02/08 00:29:47, JF wrote:
> 3 x unique_ptr?

Yeah, I think so, after Karl's CL lands...

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

https://codereview.chromium.org/876083007/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:407: Ice::VariableDeclarationList *VariableDeclarations;
On 2015/02/08 00:29:47, JF wrote:
> Why this change?
> 
> Also, unique_ptr.

It was changed because this is the part that gets split off and sent to the
emitter thread.  In some future implementation, the translator object could
potentially be deleted before the emitter thread completes, so this needs to be
detached.

I'll take care of the unique_ptr stuff after Karl's CL lands.

https://codereview.chromium.org/876083007/diff/40001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/876083007/diff/40001/src/IceCfg.h#newcode32
src/IceCfg.h:32: 
On 2015/02/08 21:11:57, Karl wrote:
> Why not SequenceNumber?

Done.

https://codereview.chromium.org/876083007/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/876083007/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:221: VariableDeclarationList *VariableDeclarations,
On 2015/02/08 21:11:57, Karl wrote:
> Add a TODO here to change this to unique_ptr once VariableDeclarations is
> parallelized (you can use my name, since I will fix it).

Actually I'll take care the last of the unique_ptr plumbing after your related
CL lands.

https://codereview.chromium.org/876083007/diff/40001/src/IceThreading.h
File src/IceThreading.h (right):

https://codereview.chromium.org/876083007/diff/40001/src/IceThreading.h#newco...
src/IceThreading.h:61: : std::min(MaxSize, MaxStaticSize)),
On 2015/02/08 21:15:04, JF wrote:
> Why have UnlimitedSize at all when MaxStaticSize would do the same?

Oh, right!  Done.

https://codereview.chromium.org/876083007/diff/40001/src/IceThreading.h#newco...
src/IceThreading.h:154: class EmitterWorkItem {
On 2015/02/08 21:15:05, JF wrote:
> Overall this class looks like it wants to be a virtual class (with all the
> different ctors and all) but is instead the union of all the derived class'
> functionality. IIUC you want it all inline without abstraction to ensure that
it
> can be held in datastructures without indirection and extra allocations?
> 
> That's fine with me, I just want to make sure that's intentional, and if so
you
> should document the design choice on the class.

In some world, I would have Cfg, Assembler, and VariableDeclarationList all
inheriting from the EmitterWorkItem interface.  EmitterWorkItem would hold the
Sequence and Kind fields (or Kind would be separately derived from some RTTI
mechanism), and the appropriate get() methods would just be static casts.

Or, EmitterWorkItem could be a tagged union plus the Sequence field, perhaps
with a nice destructor defined in the tagged union.

These ideas have a certain ickiness that led to the slightly wasteful approach
here, where at most one of the pointer fields is non-null.

I don't care too much about inline versus non-inline since there are relatively
few operations, but it's simple enough that it's nice to define in just one
place.

Added some documentation above.

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/arith-opt.ll (right):

https://codereview.chromium.org/876083007/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/arith-opt.ll:6: ; RUN: %p2i -i %s --args --verbose inst
-threads=0 -ias=0 | FileCheck %s
On 2015/02/08 21:11:57, Karl wrote:
> Is -threades=0 necessary (see run-llvm2ice.py).

[This is an answer to the same question across many .ll files.]

If the test runs "%p2i --insts", then -threads=0 gets added automatically.  But
if the RUN line includes "--args -verbose inst" and does not include "--insts",
then -threads=0 needs to be added explicitly.

It's tempting to change the RUN line here to something like:

RUN: %p2i -i %s --insts --args -ias=0 | FileCheck %s

However, it turns out this test is actually testing some combination of dump and
emit output, and "--insts" adds the "-notranslate" option which removes emit
output.

[Jim Stichnoth, 2015-02-10 07:55:00]

After Karl's global initializer CL lands, and also given the recent Flags
change, I'm pretty sure the rebase will add a lot of distractions to this CL. 
So I'm planning to close this CL, do the painful rebase, and create a new CL. 
I'll address any new comments in the meantime, but I'm not sure yet which CL (or
both) will have the changes.

[Jim Stichnoth, 2015-02-11 05:46:37]

Closing this CL, continuing in https://codereview.chromium.org/916653004/ .