Refactor Subzero initialization and add a browser callback handler.

Refactor Subzero initialization and add a browser callback handler.

Handlers are represented as a "compile server" even though
right now it can really only handle a single
compile request.

Then there can be a commandline-based server and a
browser-based server. This server takes over the main
thread. In the browser-based case the server can block,
waiting on bytes to be pushed. This becomes a producer of
bitcode bytes.

The original main thread which did bitcode reading is now
shifted to yet another worker thread, which is then the
consumer of bitcode bytes.

This uses an IRT interface for listening to messages
from the browser:
https://codereview.chromium.org/984713003/

TEST=Build the IRT core nexe w/ the above patch and compile w/ something like:

echo """
readwrite_file objfile /tmp/temp.nexe---gcc.opt.stripped.pexe---.o
rpc StreamInitWithSplit i(4) h(objfile) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) C(4,-O2\x00) * s()
stream_file /usr/local/google/home/jvoung/pexe_tests/gcc.opt.stripped.pexe 65536 1000000000
rpc StreamEnd * i() s() s() s()
echo "pnacl-sz complete"
""" | scons-out/opt-linux-x86-32/staging/sel_universal \
    -a -B scons-out/nacl_irt-x86-32/staging/irt_core.nexe \
    --abort_on_error \
    -- toolchain/linux_x86/pnacl_translator/translator/x86-32/bin/pnacl-sz.nexe

echo """
readwrite_file nexefile /tmp/temp.nexe.tmp
readonly_file objfile0 /tmp/temp.nexe---gcc.opt.stripped.pexe---.o
rpc RunWithSplit i(1) h(objfile0) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(nexefile) *
echo "ld complete"
""" | /usr/local/google/home/nacl3/native_client/scons-out/opt-linux-x86-32/staging/sel_universal \
    --abort_on_error \
    -a -B \
    scons-out/nacl_irt-x86-32/staging/irt_core.nexe \
    -E NACL_IRT_OPEN_RESOURCE_BASE=toolchain/linux_x86/pnacl_translator/translator/x86-32/lib/ \
    -E NACL_IRT_OPEN_RESOURCE_REMAP=libpnacl_irt_shim.a:libpnacl_irt_shim_dummy.a \
    -- toolchain/linux_x86/pnacl_translator/translator/x86-32/bin/ld.nexe

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4091
R=kschimpf@google.com, stichnot@chromium.org

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

[jvoung (off chromium), 2015-03-13 01:16:26]

Patchset #1 (id:1) has been deleted

[jvoung (off chromium), 2015-03-13 01:16:33]

Patchset #1 (id:20001) has been deleted

[jvoung (off chromium), 2015-03-13 01:16:40]

Patchset #1 (id:40001) has been deleted

[jvoung (off chromium), 2015-03-13 01:16:52]

Patchset #2 (id:80001) has been deleted

[jvoung (off chromium), 2015-03-13 02:07:09]

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

[jvoung (off chromium), 2015-03-13 02:07:52]

Still a bunch of cleanup I want to do, but this is the rough first "working"
version, in case you want to take a quick look and have high-level comments at
the approach.

https://codereview.chromium.org/997773002/diff/180001/src/IceCompiler.h
File src/IceCompiler.h (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceCompiler.h#newco...
src/IceCompiler.h:36: void setReturnValue(int V) { ReturnValue = V; }
might stash / pull from the global context instead, but haven't set that up yet

[Jim Stichnoth, 2015-03-13 04:55:00]

A few minor comments...

https://codereview.chromium.org/997773002/diff/180001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/997773002/diff/180001/Makefile.standalone#new...
Makefile.standalone:26: NACL_ROOT= $(shell readlink -e ../../../)
Maybe we can do something like this?

python -c "import sys; sys.path.insert(0, 'pydir'); import utils; print
utils.FindBaseNaCl()"

or maybe there's a more attractive way of doing it.

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:18: #include "IceBrowserCompileServer.h"
Move after llvm/ includes?

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:26: #include "llvm/Support/raw_os_ostream.h"
sort these includes

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.h:40: BrowserCompileServer(Compiler &Comp)
explicit

https://codereview.chromium.org/997773002/diff/180001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:14: #include "IceCompileServer.h"
Move this after the llvm/ includes?

https://codereview.chromium.org/997773002/diff/180001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:18: #include "IceCompiler.h"
Can this be sorted in the usual order?

[Mircea Trofin, 2015-03-13 22:01:34]

https://codereview.chromium.org/997773002/diff/200001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/200001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:101: struct PNaClSubzeroFunctions
SubzeroCallbacks {
are we still using the subzero name?

https://codereview.chromium.org/997773002/diff/200001/src/IceCompiler.h
File src/IceCompiler.h (right):

https://codereview.chromium.org/997773002/diff/200001/src/IceCompiler.h#newco...
src/IceCompiler.h:31: void run(int argc, char **argv, CompileServer &Server);
Could argument passing happen before run is called? That way, for repeated
compiles taking the same arguments, just different input source, arg parsing can
be up-fronted.

[Jim Stichnoth, 2015-03-14 02:31:05]

https://codereview.chromium.org/997773002/diff/200001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/200001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:101: struct PNaClSubzeroFunctions
SubzeroCallbacks {
On 2015/03/13 22:01:34, Mircea Trofin wrote:
> are we still using the subzero name?

Yeah, Subzero (maybe abbreviated sz) is the "official" name.  Earlier I changed
the no-longer-appropriate uses of llvm2ice.

[Mircea Trofin, 2015-03-14 03:20:00]

thanks! I must have missed that
On Fri, Mar 13, 2015 at 19:31 <stichnot@chromium.org> wrote:

>
> https://codereview.chromium.org/997773002/diff/200001/src/
> IceBrowserCompileServer.cpp
> File src/IceBrowserCompileServer.cpp (right):
>
> https://codereview.chromium.org/997773002/diff/200001/src/
> IceBrowserCompileServer.cpp#newcode101
> src/IceBrowserCompileServer.cpp:101
>
<https://codereview.chromium.org/997773002/diff/200001/src/IceBrowserCompileSe...>:
> struct PNaClSubzeroFunctions
> SubzeroCallbacks {
> On 2015/03/13 22:01:34, Mircea Trofin wrote:
> > are we still using the subzero name?
>
> Yeah, Subzero (maybe abbreviated sz) is the "official" name.  Earlier I
> changed the no-longer-appropriate uses of llvm2ice.
>
> https://codereview.chromium.org/997773002/
>

-- 
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 http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Karl, 2015-03-16 16:09:29]

LGTM

[jvoung (off chromium), 2015-03-18 15:39:09]

https://codereview.chromium.org/997773002/diff/180001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/997773002/diff/180001/Makefile.standalone#new...
Makefile.standalone:26: NACL_ROOT= $(shell readlink -e ../../../)
On 2015/03/13 04:55:00, stichnot wrote:
> Maybe we can do something like this?
> 
> python -c "import sys; sys.path.insert(0, 'pydir'); import utils; print
> utils.FindBaseNaCl()"
> 
> or maybe there's a more attractive way of doing it.

Done.

Another similar way would be to outline the snippet to a tiny wrapper script
like "pydir/find_nacl_base.py".

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:18: #include "IceBrowserCompileServer.h"
On 2015/03/13 04:55:00, stichnot wrote:
> Move after llvm/ includes?

Done.

Oops, I got mixed up with some other coding styles that would put the Foo.h
first in each Foo.cpp.

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:26: #include "llvm/Support/raw_os_ostream.h"
On 2015/03/13 04:55:00, stichnot wrote:
> sort these includes

Done.

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:29: #include <irt.h>
I moved these to be with the other <...> includes. Though it's not part of the
c++/c standard libraries, it does come with the toolchain.

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.h:40: BrowserCompileServer(Compiler &Comp)
On 2015/03/13 04:55:00, stichnot wrote:
> explicit

Done.

https://codereview.chromium.org/997773002/diff/180001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:14: #include "IceCompileServer.h"
On 2015/03/13 04:55:00, stichnot wrote:
> Move this after the llvm/ includes?

Done.

https://codereview.chromium.org/997773002/diff/180001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/997773002/diff/180001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:18: #include "IceCompiler.h"
On 2015/03/13 04:55:00, stichnot wrote:
> Can this be sorted in the usual order?

Done.

https://codereview.chromium.org/997773002/diff/200001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/200001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:101: struct PNaClSubzeroFunctions
SubzeroCallbacks {
On 2015/03/14 02:31:05, stichnot wrote:
> On 2015/03/13 22:01:34, Mircea Trofin wrote:
> > are we still using the subzero name?
> 
> Yeah, Subzero (maybe abbreviated sz) is the "official" name.  Earlier I
changed
> the no-longer-appropriate uses of llvm2ice.

Acknowledged.

https://codereview.chromium.org/997773002/diff/200001/src/IceCompiler.h
File src/IceCompiler.h (right):

https://codereview.chromium.org/997773002/diff/200001/src/IceCompiler.h#newco...
src/IceCompiler.h:31: void run(int argc, char **argv, CompileServer &Server);
On 2015/03/13 22:01:34, Mircea Trofin wrote:
> Could argument passing happen before run is called? That way, for repeated
> compiles taking the same arguments, just different input source, arg parsing
can
> be up-fronted.
> 

Hmm, good point.

There is an Ice::ClFlags class that holds *most* of the flags. We could parse
immediately before running, set up a ClFlags object, and then only pass the
ClFlags object to run.

However, there are a bunch of flags not captured by the ClFlags object. There
could be a couple reasons (a) they are not checked by the downstream code so
don't need to be so global and (b) they are also not needed by the "release"
build.  I could create a separate class, Ice::ClFlagsExtra or something... Then
do run(Flags, FlagsExtra, Server).

There is also this related issue
https://code.google.com/p/nativeclient/issues/detail?id=4084, but I think that's
a separate change. The run(Flags, FlagsExtra, ...) structure may help with
that...

Some other things -- besides changing the input/output files across compiles, we
may also want to change the OptLevel or the RNG seed flags between compiles.

Thoughts?

[Mircea Trofin, 2015-03-18 16:51:06]

https://codereview.chromium.org/997773002/diff/200001/src/IceCompiler.h
File src/IceCompiler.h (right):

https://codereview.chromium.org/997773002/diff/200001/src/IceCompiler.h#newco...
src/IceCompiler.h:31: void run(int argc, char **argv, CompileServer &Server);
On 2015/03/18 15:39:09, jvoung wrote:
> On 2015/03/13 22:01:34, Mircea Trofin wrote:
> > Could argument passing happen before run is called? That way, for repeated
> > compiles taking the same arguments, just different input source, arg parsing
> can
> > be up-fronted.
> > 
> 
> Hmm, good point.
> 
> There is an Ice::ClFlags class that holds *most* of the flags. We could parse
> immediately before running, set up a ClFlags object, and then only pass the
> ClFlags object to run.
> 
> However, there are a bunch of flags not captured by the ClFlags object. There
> could be a couple reasons (a) they are not checked by the downstream code so
> don't need to be so global and (b) they are also not needed by the "release"
> build.  I could create a separate class, Ice::ClFlagsExtra or something...
Then
> do run(Flags, FlagsExtra, Server).
> 
> There is also this related issue
> https://code.google.com/p/nativeclient/issues/detail?id=4084, but I think
that's
> a separate change. The run(Flags, FlagsExtra, ...) structure may help with
> that...
> 
> Some other things -- besides changing the input/output files across compiles,
we
> may also want to change the OptLevel or the RNG seed flags between compiles.
> 
> Thoughts?

I think the 2 issues are indeed related. I believe a programmatic  API for the
compiler would address them both. 

I think it would be reasonable to separate the 2 changes, though (meaning, do
the API design separately). Issue 4048 seems like the right place to peg that
effort off.

[Mircea Trofin, 2015-03-18 16:56:01]

one more nit - can we automate the testing somehow?

Thanks!

[jvoung (off chromium), 2015-03-20 23:53:31]

Patchset #10 (id:260001) has been deleted

[jvoung (off chromium), 2015-03-20 23:53:42]

Patchset #10 (id:280001) has been deleted

[jvoung (off chromium), 2015-03-20 23:53:51]

Patchset #10 (id:300001) has been deleted

[jvoung (off chromium), 2015-03-20 23:54:02]

Patchset #11 (id:340001) has been deleted

[jvoung (off chromium), 2015-03-25 05:22:11]

Some diffs from earlier CL:

- Added Makefile.standalone target "sb" for more quickly testing that the
#ifdef'ed irt-dependent code compiles w/out going through the llvm Makefiles. I
didn't make this part of "all" because (a) it's slow, and (b) it depends on the
irt-dev.h CL to actually build.

- Moved GlobalContext creation outside of the parser thread / compiler::run
thread into the server thread, and changed compiler::run to set the context's
error code. This is instead of keeping yet another a copy of the error status in
the Ice::Compiler. The server thread join()s and then checks the GlobalContext's
error code.

- Because GlobalContext is constructed on the server thread, its thread local
storage / ThreadContext is no longer valid on the new parser thread unless it's
initialized... so I initialize that TLS on the parser thread after creating the
context on the server thread. Seems a bit non-idiomatic compared to the existing
startWorkerThreads() since TLS init is separate from thread creation, but I
guess it works.

- Removed the getInputStream(filename)/getOutputStream(filename) callbacks and
replaced it with just (a) passing the InputStream to run() as a parameter (b)
passing the context to run(), and the context already holds the output and log
streams.

- Ended up getting rid of argc/argv from the run() parameters. Move the original
main() specific flags to ClFlagsExtra. Pass that into run() also. The original
ClFlags are part of the Context as it always has been, so that gets passed along
to run() along with the Context.

- I'm not too happy with having both IceCompiler.h and IceTranslator.h since
that seems to distribute the "driver for translation" code between the two
files... Also IceCompiler is pretty slim now so it's not clear it needs to be
separate. I thought about moving the run() to be a static method of
IceTranslator.h.  The weird thing is that run() depends on PNaClTranslator.h,
which derives from the Ice::Translator class. I guess that's not circular in
terms of .h includes, since it's the IceTranslator.cpp which will need
PNaClTranslator.h for run(), if run() is moved to IceTranslator.h.

- As for the question of automated testing -- this depends on some external
files being built -- the irt_core.nexe, sel_ldr, sel_universal, and
toolchain_build_pnacl.py --build-sbtc sandboxed_translators libraries. It also
depends on the "readwrite_file objfile foo.o
rpc StreamInitWithSplit i(4) h(objfile) h(invalid) h(invalid) h(invalid)
h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) h(invalid) ..." stuff,
which the pnacl-translate.py already knows how to generate. So... 

(a) could wait till I add pnacl-translate.py support for the NaCl pnacl-sz.nexe.
 Jim recently added the unsandboxed pnacl-sz, and I can add the extra bits for
pnacl-sz.nexe.  Then we can depend on pnacl-translate.py and make a mode of
cross tests which use pnacl-translate.py w/ the nexe translator. However it
might be a different/optional cross-test target since there are so many
dependencies.

(b) make some standalone version of the of the "readwrite_file objfile ..."
orchestration of sel_universal to not depend on pnacl-translate.py... not a huge
fan of duplicating that... it only eliminates one of many dependencies for the
standalone build.

- Still TODO: be able to return error messages without terminating the
server_thread and only terminate worker threads. Would like to automate testing
for that part too... since it's easy to make a mistake and get the whole thing
to hang on error instead of terminating.

[Jim Stichnoth, 2015-03-25 16:34:06]

Otherwise lgtm.

As for testing, I think it's probably more appropriate for the integration tests
to be outside the src/subzero repo, as opposed to the Makefile.standalone which
makes fairly minimal requirements on external tools/apparatus.

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:31: NACL_ROOT ?= $(shell python -c "import sys;
sys.path.insert(0, 'pydir'); import utils; print utils.FindBaseNaCl()")
Can this line be broken up for 80-col?

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:122: # sandboxed_translators (which doens't include every
library
doesn't

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:133: # It's recommended that CXX matches the compiler you
used to build LLVM itself.
This is a really old comment, and not really valid now that we're using the
fixed third-party compiler.  Remove it?

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:201: ifndef TSAN
Change to
  ifdef ... else ... endif
instead of starting with ifndef?

https://codereview.chromium.org/997773002/diff/400001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/400001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:1: //===-
subzero/src/IceBrowserCompileServer.cpp - Browser Compile server ---===//
lowercase compile?

https://codereview.chromium.org/997773002/diff/400001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:42: llvm::report_fatal_error("Failed to get
translator_subzero IRT interface");
Is "translator_subzero" still an accurate description here?

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:23: namespace cl = llvm::cl;
Move this to be above the comment above?  Since namespace applies to both sets
of options.

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:25: static cl::opt<bool> AllowErrorRecovery(
Can change all these from being static to being inside an anonymous namespace?

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:195: "Use all verbose options except 'regalloc' and 'time'"),
Oops, there's no longer a "time" option, can you remove that from this help
text?

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:242: static IceString AppName;
Don't use static global variables - either make "global" inside namespace Ice,
or put inside an anonymous namespace.

https://codereview.chromium.org/997773002/diff/400001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/997773002/diff/400001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:304: ThreadContext *TLS = new ThreadContext();
I guess this gets renamed to Tls after
https://codereview.chromium.org/1030793002/ ?

https://codereview.chromium.org/997773002/diff/400001/src/main.cpp
File src/main.cpp (right):

https://codereview.chromium.org/997773002/diff/400001/src/main.cpp#newcode31
src/main.cpp:31: #else
#else // !PNACL_BROWSER_TRANSLATOR
...
#endif // !PNACL_BROWSER_TRANSLATOR

(and yeah, now I see that a few more are missing in IceOperand.cpp and
IceTargetLowering.cpp and assembler_ia32.cpp...)

[jvoung (off chromium), 2015-03-25 18:14:35]

Thanks!

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:31: NACL_ROOT ?= $(shell python -c "import sys;
sys.path.insert(0, 'pydir'); import utils; print utils.FindBaseNaCl()")
On 2015/03/25 16:34:06, stichnot wrote:
> Can this line be broken up for 80-col?

Done.

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:122: # sandboxed_translators (which doens't include every
library
On 2015/03/25 16:34:06, stichnot wrote:
> doesn't

Done.

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:133: # It's recommended that CXX matches the compiler you
used to build LLVM itself.
On 2015/03/25 16:34:06, stichnot wrote:
> This is a really old comment, and not really valid now that we're using the
> fixed third-party compiler.  Remove it?

Done.

https://codereview.chromium.org/997773002/diff/400001/Makefile.standalone#new...
Makefile.standalone:201: ifndef TSAN
On 2015/03/25 16:34:06, stichnot wrote:
> Change to
>   ifdef ... else ... endif
> instead of starting with ifndef?

Done.

https://codereview.chromium.org/997773002/diff/400001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/997773002/diff/400001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:1: //===-
subzero/src/IceBrowserCompileServer.cpp - Browser Compile server ---===//
On 2015/03/25 16:34:06, stichnot wrote:
> lowercase compile?

Done.

https://codereview.chromium.org/997773002/diff/400001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:42: llvm::report_fatal_error("Failed to get
translator_subzero IRT interface");
On 2015/03/25 16:34:06, stichnot wrote:
> Is "translator_subzero" still an accurate description here?

Done.

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:23: namespace cl = llvm::cl;
On 2015/03/25 16:34:06, stichnot wrote:
> Move this to be above the comment above?  Since namespace applies to both sets
> of options.

Done.

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:25: static cl::opt<bool> AllowErrorRecovery(
On 2015/03/25 16:34:06, stichnot wrote:
> Can change all these from being static to being inside an anonymous namespace?

Done.

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:195: "Use all verbose options except 'regalloc' and 'time'"),
On 2015/03/25 16:34:06, stichnot wrote:
> Oops, there's no longer a "time" option, can you remove that from this help
> text?

Done.

https://codereview.chromium.org/997773002/diff/400001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:242: static IceString AppName;
On 2015/03/25 16:34:06, stichnot wrote:
> Don't use static global variables - either make "global" inside namespace Ice,
> or put inside an anonymous namespace.

Done.

https://codereview.chromium.org/997773002/diff/400001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/997773002/diff/400001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:304: ThreadContext *TLS = new ThreadContext();
On 2015/03/25 16:34:06, stichnot wrote:
> I guess this gets renamed to Tls after
> https://codereview.chromium.org/1030793002/ ?

Oops -- Done.

https://codereview.chromium.org/997773002/diff/400001/src/main.cpp
File src/main.cpp (right):

https://codereview.chromium.org/997773002/diff/400001/src/main.cpp#newcode31
src/main.cpp:31: #else
On 2015/03/25 16:34:06, stichnot wrote:
> #else // !PNACL_BROWSER_TRANSLATOR
> ...
> #endif // !PNACL_BROWSER_TRANSLATOR
> 
> (and yeah, now I see that a few more are missing in IceOperand.cpp and
> IceTargetLowering.cpp and assembler_ia32.cpp...)

Done.

[jvoung (off chromium), 2015-03-26 22:35:38]

Changed to adapt to IRT changes (ptal?)

[jvoung (off chromium), 2015-03-27 23:29:15]

Committed patchset #20 (id:540001) manually as
44c3a8046f4c81f9b4476f8c3c72723b584c312d (presubmit successful).