Allow Subzero to parse function blocks in parallel.

Allow Subzero to parse function blocks in parallel.

This CL modifies the code so that we can do sequential and parallel
parsing of function blocks in bitcode files, based on a command line
argument.

The command line argument was added because during testing, I had one
compilation failure (transient), and do not know the cause. Hence, I
was reluctant to install this CL without a command-line flag.

To test the new parallel parser, the easiest solution is to edit
IceClFlags.def and set the default value of ParseParallel to true.

This code also fixes up unit parsing tests, as well as one parsing
test.  The cause of these problems was the implicit assumption that
function blocks are parsed sequentially, which no longer applies when
function blocks are parsed in parallel. To fix this, the "threads=0"
command line argument was added. It also added the starting up of
worker threads, since parsing of function blocks will happen in the
translation thread if parallel parsing is turned on.

The OptQ queue was modified to contain OptWorkerItem instances with a
single virtual to get the parsed code. This allows the IceConverter to
continue to work, by simply passing the generated Cfg as a work item.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2016-03-23 21:34:55]

Description was changed from

==========
Allow Subzero to parse function blocks in parallel.

This CL modifies the code so that we can do sequential and parallel
parsing of function blocks in bitcode files, based on a command line
argument.

The command line argument was added because during testing, I had one
compilation failure (transient), and do not know the cause. Hence, I
was reluctant to install this CL without a command-line flag.

To test the new parallel parser, the easiest solution is to edit
IceClFlags.def and set the default value to true.

This code also fixes up a unit parsing tests, as well as one parsing
test.  The cause of these problems was the implicit assumption that
function blocks are parsed sequentially, which no longer applies when
function blocks are parsed in parallel. To fix this, the "threads=0"
command line argument was added. It also added the starting up of
worker threads, since parsing of function blocks will happen in the
translation thread if parallel parsing is turned on.

The OptQ queue was modified to contain OptWorkerItem instances with a
single virtual to get the parsed code. This allows the IceConverter to
continue to work, by simply passing the generated Cfg as a work item.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

to

==========
Allow Subzero to parse function blocks in parallel.

This CL modifies the code so that we can do sequential and parallel
parsing of function blocks in bitcode files, based on a command line
argument.

The command line argument was added because during testing, I had one
compilation failure (transient), and do not know the cause. Hence, I
was reluctant to install this CL without a command-line flag.

To test the new parallel parser, the easiest solution is to edit
IceClFlags.def and set the default value of ParseParallel to true.

This code also fixes up a unit parsing tests, as well as one parsing
test.  The cause of these problems was the implicit assumption that
function blocks are parsed sequentially, which no longer applies when
function blocks are parsed in parallel. To fix this, the "threads=0"
command line argument was added. It also added the starting up of
worker threads, since parsing of function blocks will happen in the
translation thread if parallel parsing is turned on.

The OptQ queue was modified to contain OptWorkerItem instances with a
single virtual to get the parsed code. This allows the IceConverter to
continue to work, by simply passing the generated Cfg as a work item.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

[Karl, 2016-03-23 21:36:14]

Description was changed from

==========
Allow Subzero to parse function blocks in parallel.

This CL modifies the code so that we can do sequential and parallel
parsing of function blocks in bitcode files, based on a command line
argument.

The command line argument was added because during testing, I had one
compilation failure (transient), and do not know the cause. Hence, I
was reluctant to install this CL without a command-line flag.

To test the new parallel parser, the easiest solution is to edit
IceClFlags.def and set the default value of ParseParallel to true.

This code also fixes up a unit parsing tests, as well as one parsing
test.  The cause of these problems was the implicit assumption that
function blocks are parsed sequentially, which no longer applies when
function blocks are parsed in parallel. To fix this, the "threads=0"
command line argument was added. It also added the starting up of
worker threads, since parsing of function blocks will happen in the
translation thread if parallel parsing is turned on.

The OptQ queue was modified to contain OptWorkerItem instances with a
single virtual to get the parsed code. This allows the IceConverter to
continue to work, by simply passing the generated Cfg as a work item.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

to

==========
Allow Subzero to parse function blocks in parallel.

This CL modifies the code so that we can do sequential and parallel
parsing of function blocks in bitcode files, based on a command line
argument.

The command line argument was added because during testing, I had one
compilation failure (transient), and do not know the cause. Hence, I
was reluctant to install this CL without a command-line flag.

To test the new parallel parser, the easiest solution is to edit
IceClFlags.def and set the default value of ParseParallel to true.

This code also fixes up unit parsing tests, as well as one parsing
test.  The cause of these problems was the implicit assumption that
function blocks are parsed sequentially, which no longer applies when
function blocks are parsed in parallel. To fix this, the "threads=0"
command line argument was added. It also added the starting up of
worker threads, since parsing of function blocks will happen in the
translation thread if parallel parsing is turned on.

The OptQ queue was modified to contain OptWorkerItem instances with a
single virtual to get the parsed code. This allows the IceConverter to
continue to work, by simply passing the generated Cfg as a work item.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

[Karl, 2016-03-24 17:15:02]

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

[John, 2016-03-24 17:32:35]

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:117: Ctx.waitForWorkerThreads();
I don't understand this change.

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:25: namespace {
enclose this namespace in Ice, and then remove the Ice:: references.

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:34: std::unique_ptr<Ice::Cfg> getParsedCfg() final {
return std::move(Func); }
final is better off when used in the class declaration, i.e.,

class CfgOptWorkItem final : public OptWOrkItem {};

then these "final" become "override"

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:35: ~CfgOptWorkItem() final {}
~CfgOptWorkItem() override = default;

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

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:585: ErrorStatus.assign(Ice::EC_Bitcode);
Are you using the OstreamLocker to prevent concurrent assignments to
ErrorStatus? If so, I would suggest that instead a new mutex is added to protect
it.

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1398: return std::move(Func);
no need to move it here -- Func is an xvalue here -- because of the return -- so
it will be moved. (and according to the "Effective Modern C++11" book, having
std::move here may also lead to worse code.

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2993: ~ModuleParser() final = default;
marking the destructor "final" is a pretty strong evidence that the class should
be final instead.

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3088: ~CfgParserWorkItem() final {}
make the class final instead?

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3091: unsigned BlockID;
can these be const?

[Karl, 2016-03-24 17:37:50]

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:117: Ctx.waitForWorkerThreads();
On 2016/03/24 17:32:34, John wrote:
> I don't understand this change.

For the bitcode reader, we must move this call inside the bitcode parser, so
that the module block doesn't disappear until all function blocks are parsed
(this is needed because the parse state is held in the module block parser).
Hence, we can't apply in this method for all cases.

Therefore, for the bitcode parser (lines 83-87), we don't call it. Only if the
IceConverter is called, do we call it in this method.

[John, 2016-03-24 17:44:00]

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:117: Ctx.waitForWorkerThreads();
On 2016/03/24 17:37:49, Karl wrote:
> On 2016/03/24 17:32:34, John wrote:
> > I don't understand this change.
> 
> For the bitcode reader, we must move this call inside the bitcode parser, so
> that the module block doesn't disappear until all function blocks are parsed
> (this is needed because the parse state is held in the module block parser).
> Hence, we can't apply in this method for all cases.
> 
> Therefore, for the bitcode parser (lines 83-87), we don't call it. Only if the
> IceConverter is called, do we call it in this method.

ok, maybe add a comment here? I get nervous when I look at code that does:

startThreads()

if (Cond) {
  joinThreads();
}

terminateProcessing();

[Karl, 2016-03-24 22:35:26]

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1834473002/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:117: Ctx.waitForWorkerThreads();
On 2016/03/24 17:44:00, John wrote:
> On 2016/03/24 17:37:49, Karl wrote:
> > On 2016/03/24 17:32:34, John wrote:
> > > I don't understand this change.
> > 
> > For the bitcode reader, we must move this call inside the bitcode parser, so
> > that the module block doesn't disappear until all function blocks are parsed
> > (this is needed because the parse state is held in the module block parser).
> > Hence, we can't apply in this method for all cases.
> > 
> > Therefore, for the bitcode parser (lines 83-87), we don't call it. Only if
the
> > IceConverter is called, do we call it in this method.
> 
> ok, maybe add a comment here? I get nervous when I look at code that does:
> 
> startThreads()
> 
> if (Cond) {
>   joinThreads();
> }
> 
> terminateProcessing();

I looked at the code more carefully, and (due to possible errors) the bitcode
parser can terminate in several places. To make it easier to deal with this, I
made WaitForWorkerThreads() only apply once, no matter how many times it is
called.

This also allows me to keep you general pattern here.

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:25: namespace {
On 2016/03/24 17:32:35, John wrote:
> enclose this namespace in Ice, and then remove the Ice:: references.

Done.

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:34: std::unique_ptr<Ice::Cfg> getParsedCfg() final {
return std::move(Func); }
On 2016/03/24 17:32:34, John wrote:
> final is better off when used in the class declaration, i.e.,
> 
> class CfgOptWorkItem final : public OptWOrkItem {};
> 
> then these "final" become "override"

https://codereview.chromium.org/1834473002/diff/20001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:35: ~CfgOptWorkItem() final {}
On 2016/03/24 17:32:35, John wrote:
> ~CfgOptWorkItem() override = default;

Done.

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

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:434: unsigned NumErrors = 0;
Removed this field. It is no longer used.

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:585: ErrorStatus.assign(Ice::EC_Bitcode);
On 2016/03/24 17:32:35, John wrote:
> Are you using the OstreamLocker to prevent concurrent assignments to
> ErrorStatus? If so, I would suggest that instead a new mutex is added to
protect
> it.

Yes I was using the OstreamLocker. I did it because this is the only place where
these variables get updated. However, I agree that is a dangerous choice in that
others may not note. Changing.

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1398: return std::move(Func);
On 2016/03/24 17:32:35, John wrote:
> no need to move it here -- Func is an xvalue here -- because of the return --
so
> it will be moved. (and according to the "Effective Modern C++11" book, having
> std::move here may also lead to worse code.

The compiler disagrees with you!

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2993: ~ModuleParser() final = default;
On 2016/03/24 17:32:35, John wrote:
> marking the destructor "final" is a pretty strong evidence that the class
should
> be final instead.

Moved final keywords (where appropriate) to the class.

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3088: ~CfgParserWorkItem() final {}
On 2016/03/24 17:32:35, John wrote:
> make the class final instead?

Done.

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3091: unsigned BlockID;
On 2016/03/24 17:32:35, John wrote:
> can these be const?

Done, except that ModParser can't be constant (Constructor FunctionParser()
requires it.

[Jim Stichnoth, 2016-03-25 04:31:04]

https://codereview.chromium.org/1834473002/diff/40001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1834473002/diff/40001/src/IceClFlags.def#newc...
src/IceClFlags.def:195: cl::desc("Parse function blocks in parallel"),
cl::init(false))            \
I think the default value should be true, otherwise it really won't get tested.

https://codereview.chromium.org/1834473002/diff/40001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1834473002/diff/40001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:66: Ctx.getErrorStatus()->assign(EC_Args);
I'm curious why this line got moved?

https://codereview.chromium.org/1834473002/diff/40001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:67: // TODO(stichnot): Access the actual command-line
argument via
Can you just delete this TODO?  Pretty sure it's not gonna happen.

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

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:223: std::unique_lock<GlobalLockType>
L(WaitForWorkersLock);
Our new super-awesome coding style would name this object "_" instead of "L".

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:224: if (WaitForWorkerThreadsCalled)
It seems that WaitForWorkerThreadsCalled could actually be a std::atomic_bool
and you could use std::atomic::exchange to manipulate it here, and avoid the
mutex altogether.

I would either add a TODO, or just go ahead and make the change.

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:92: OptWorkItem() {}
OptWorkItem() = default;
?

Same for ~OptWorkItem() ?

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:553: // True if WaitForWorkerThreads have been called.
True if waitForWorkerThreads() has been called.

https://codereview.chromium.org/1834473002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1834473002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:583: std::unique_lock<Ice::GlobalLockType>
L(ErrorReportingLock);
name this "_" instead of "L"?

https://codereview.chromium.org/1834473002/diff/40001/unittest/BitcodeMunge.cpp
File unittest/BitcodeMunge.cpp (right):

https://codereview.chromium.org/1834473002/diff/40001/unittest/BitcodeMunge.c...
unittest/BitcodeMunge.cpp:30: Flags.setVerbose(Ice::IceV_None);
There is already a setVerbose 2 lines above.  The default is IceV_None, so
remove both?

[John, 2016-03-25 04:55:22]

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

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1398: return std::move(Func);
Ah, Func is a class member... That's one of the worst features the stupid llvm
coding style... <rant />

Does Func need to be a class member? It seems it doesn't. If it doesn't, make it
a local -- then std::move is not needed.

[Karl, 2016-03-29 17:35:02]

There was still threading issues in the previous version of this CL. The cause
was that the memory object holding the input was based on a data streamer.

That is, the memory object had an internal buffer of type std::vector<uint8>
that was filled (lazily as needed) from the data streamer. The problem was that
when the vector had to grow, parallel parsers (reading the buffer) would be
trying to copy the contents of vector during copying.

This showed that the memory object of the top-level parser is not thread safe.
To fix this, I modified the top-level parser to copy the bytes associated with
each memory block, and create a separate memory object (and bitstream reader) to
parse the buffered bytes.

One problem remaining that is not being dealt with in this CL is error reporting
of bit-position within parallel parses. The code to compute error positions is
wrong because the readers of the parallel parsers no longer know the start bit
position. This will be added as a later CL.

The current workaround to finding the location of bitcode errors is to either
(1) parse sequentially, or (2) use pnacl-bcdis to show where errors are.

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

https://codereview.chromium.org/1834473002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1398: return std::move(Func);
On 2016/03/25 04:55:22, John wrote:
> Ah, Func is a class member... That's one of the worst features the stupid llvm
> coding style... <rant />
> 
> Does Func need to be a class member? It seems it doesn't. If it doesn't, make
it
> a local -- then std::move is not needed.

It uses many instance fields and methods of the FunctionParser, so yes, it
should be a class member.

https://codereview.chromium.org/1834473002/diff/40001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1834473002/diff/40001/src/IceClFlags.def#newc...
src/IceClFlags.def:195: cl::desc("Parse function blocks in parallel"),
cl::init(false))            \
On 2016/03/25 04:31:04, stichnot wrote:
> I think the default value should be true, otherwise it really won't get
tested.

I intentionally set this to false, so that we don't break existing users, nor
will it get backed into Chrome, since it wasn't tested enough. However, I guess
we can change back before the next release if necessary. Changing.

https://codereview.chromium.org/1834473002/diff/40001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1834473002/diff/40001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:66: Ctx.getErrorStatus()->assign(EC_Args);
On 2016/03/25 04:31:04, stichnot wrote:
> I'm curious why this line got moved?

Programmer error. I initially thought I needed to lock state before generating
the error, but then realized we don't start worker threads till after this
point. So I tried (unsuccessfully) to revert the change.

Changing back.

https://codereview.chromium.org/1834473002/diff/40001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:67: // TODO(stichnot): Access the actual command-line
argument via
On 2016/03/25 04:31:04, stichnot wrote:
> Can you just delete this TODO?  Pretty sure it's not gonna happen.

Done.

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

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:223: std::unique_lock<GlobalLockType>
L(WaitForWorkersLock);
On 2016/03/25 04:31:04, stichnot wrote:
> Our new super-awesome coding style would name this object "_" instead of "L".

Removed, went with atomic_bool.

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:224: if (WaitForWorkerThreadsCalled)
On 2016/03/25 04:31:04, stichnot wrote:
> It seems that WaitForWorkerThreadsCalled could actually be a std::atomic_bool
> and you could use std::atomic::exchange to manipulate it here, and avoid the
> mutex altogether.
> 
> I would either add a TODO, or just go ahead and make the change.

Changed to atomic_bool.

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:92: OptWorkItem() {}
On 2016/03/25 04:31:04, stichnot wrote:
> OptWorkItem() = default;
> ?
> 
> Same for ~OptWorkItem() ?

Done.

https://codereview.chromium.org/1834473002/diff/40001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:553: // True if WaitForWorkerThreads have been called.
On 2016/03/25 04:31:04, stichnot wrote:
> True if waitForWorkerThreads() has been called.

Done.

https://codereview.chromium.org/1834473002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1834473002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:583: std::unique_lock<Ice::GlobalLockType>
L(ErrorReportingLock);
On 2016/03/25 04:31:04, stichnot wrote:
> name this "_" instead of "L"?

Done.

https://codereview.chromium.org/1834473002/diff/40001/unittest/BitcodeMunge.cpp
File unittest/BitcodeMunge.cpp (right):

https://codereview.chromium.org/1834473002/diff/40001/unittest/BitcodeMunge.c...
unittest/BitcodeMunge.cpp:30: Flags.setVerbose(Ice::IceV_None);
On 2016/03/25 04:31:04, stichnot wrote:
> There is already a setVerbose 2 lines above.  The default is IceV_None, so
> remove both?

Good point. Removing.

https://codereview.chromium.org/1834473002/diff/40001/unittest/BitcodeMunge.c...
unittest/BitcodeMunge.cpp:43: Translator.translateBuffer(BufferName,
MungedInput.get());
To be safe, added Ctx.waitForWorkerThreads() here.

[Karl, 2016-03-31 14:50:26]

Ping?

[John, 2016-03-31 15:51:36]

lgtm

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

https://codereview.chromium.org/1834473002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:347: std::unique_ptr<Cfg> Func =
OptItem->getParsedCfg();
optional: auto

https://codereview.chromium.org/1834473002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1834473002/diff/80001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3146: uint64_t EndBit = Cursor.GetCurrentBitNo();
can this be const?

[Jim Stichnoth, 2016-03-31 15:56:57]

Could you rebase to master so I can try it out?  I'm getting conflicts when I
"git cl patch".

[native-client-reviews_googlegroups.com, 2016-03-31 16:00:14]

I can't check that in because I still can't build the pnacl-toolchain. It
appears that I have problems with incompatable python driver code. On
script is passing '-no-finalize' to pnacl-ld.py, but pnacl-ld.py no longer
accepts it.

However, as far as I can tell, this inconsistency doesn't appear in the
pnacl/driver directory.

When I get a successful build, I will update.

Karl Schimpf

On Thu, Mar 31, 2016 at 8:56 AM, <stichnot@chromium.org> wrote:

> Could you rebase to master so I can try it out? I'm getting conflicts when
> I
> "git cl patch".
>
> https://codereview.chromium.org/1834473002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2016-03-31 16:08:09]

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

https://codereview.chromium.org/1834473002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:244: // Do a separate loop over AllThreadContexts to
avoid holding two locks
reflow...

https://codereview.chromium.org/1834473002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1834473002/diff/80001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3102: FunctionParser Parser(BlockID,
const_cast<ModuleParser *>(ModParser), FcnId,
Do you really need that const_cast here?  Couldn't you just remove const from
the declaration of the private field?

[Karl, 2016-03-31 16:33:49]

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

https://codereview.chromium.org/1834473002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:244: // Do a separate loop over AllThreadContexts to
avoid holding two locks
On 2016/03/31 16:08:08, stichnot wrote:
> reflow...

Done.

https://codereview.chromium.org/1834473002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:347: std::unique_ptr<Cfg> Func =
OptItem->getParsedCfg();
On 2016/03/31 15:51:36, John wrote:
> optional: auto

Done.

https://codereview.chromium.org/1834473002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1834473002/diff/80001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3102: FunctionParser Parser(BlockID,
const_cast<ModuleParser *>(ModParser), FcnId,
On 2016/03/31 16:08:09, stichnot wrote:
> Do you really need that const_cast here?  Couldn't you just remove const from
> the declaration of the private field?

I added this (and the necessary const_cast) at John's suggestion. Will remove.

https://codereview.chromium.org/1834473002/diff/80001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:3146: uint64_t EndBit = Cursor.GetCurrentBitNo();
On 2016/03/31 15:51:36, John wrote:
> can this be const?

Done.

[Jim Stichnoth, 2016-03-31 17:18:46]

LGTM.  Some nice observable speedups in parallel translation time!

[Karl, 2016-03-31 17:20:26]

Description was changed from

==========
Allow Subzero to parse function blocks in parallel.

This CL modifies the code so that we can do sequential and parallel
parsing of function blocks in bitcode files, based on a command line
argument.

The command line argument was added because during testing, I had one
compilation failure (transient), and do not know the cause. Hence, I
was reluctant to install this CL without a command-line flag.

To test the new parallel parser, the easiest solution is to edit
IceClFlags.def and set the default value of ParseParallel to true.

This code also fixes up unit parsing tests, as well as one parsing
test.  The cause of these problems was the implicit assumption that
function blocks are parsed sequentially, which no longer applies when
function blocks are parsed in parallel. To fix this, the "threads=0"
command line argument was added. It also added the starting up of
worker threads, since parsing of function blocks will happen in the
translation thread if parallel parsing is turned on.

The OptQ queue was modified to contain OptWorkerItem instances with a
single virtual to get the parsed code. This allows the IceConverter to
continue to work, by simply passing the generated Cfg as a work item.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

to

==========
Allow Subzero to parse function blocks in parallel.

This CL modifies the code so that we can do sequential and parallel
parsing of function blocks in bitcode files, based on a command line
argument.

The command line argument was added because during testing, I had one
compilation failure (transient), and do not know the cause. Hence, I
was reluctant to install this CL without a command-line flag.

To test the new parallel parser, the easiest solution is to edit
IceClFlags.def and set the default value of ParseParallel to true.

This code also fixes up unit parsing tests, as well as one parsing
test.  The cause of these problems was the implicit assumption that
function blocks are parsed sequentially, which no longer applies when
function blocks are parsed in parallel. To fix this, the "threads=0"
command line argument was added. It also added the starting up of
worker threads, since parsing of function blocks will happen in the
translation thread if parallel parsing is turned on.

The OptQ queue was modified to contain OptWorkerItem instances with a
single virtual to get the parsed code. This allows the IceConverter to
continue to work, by simply passing the generated Cfg as a work item.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2016-03-31 17:20:27]

Committed patchset #6 (id:100001) manually as
e8457a26638d4d3ea5420ff80ff4ad9fc1ea1fb0 (presubmit successful).