Add a switch for using PNaCl Subzero and use it for -O0 translation.

Add a switch for using PNaCl Subzero and use it for -O0 translation.

Toggles use of Subzero for x86-32 when given the
--enable-pnacl-subzero flag (bikeshedding welcome).
Is not used when debug metadata is present.
Subzero O2 is approximately LLC O0 so adjust the optlevels.
There isn't a way to select Subzero O0.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4091
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_dbg_32_ng,linux_nacl_sdk_build
TEST=
(1) build 32-bit chrome under 64-bit kernel but a 32-bit linux container and install 64-bit libc/libstdc++.
(2) xvfb-run --server-args="-screen 0 1024x768x24"  out/Release/browser_tests --gtest_filter=*PnaclSubzero*

Committed: https://crrev.com/58bea9692855603927912636d029586a90d60452
Cr-Commit-Position: refs/heads/master@{#323063}

[jvoung (off chromium), 2015-03-27 17:01:51]

jvoung@chromium.org changed reviewers:
+ dschuff@chromium.org, ncbray@chromium.org, stichnot@chromium.org

[jvoung (off chromium), 2015-03-27 17:03:24]

ncbray for nacl_browser_test*

Jim and Derek for everything else

[Jim Stichnoth, 2015-03-27 20:27:55]

Quick sanity check question.  I tried looking through this CL and the other 2
referenced CLs, but I'm not sure where the pnacl-sz arglist is being
constructed.  I wanted to verify that -threads=<N> is always being passed.

Also, I was thinking about Subzero and -threads.  We haven't looked much at
scalability yet except to observe that it's well below what we expected.  In
fact, it seems that -threads=0 (fully sequential) is faster than -threads=1
(total of 3 threads).  Probably -threads=2 is faster than -threads=0, and
performance does get marginally better as -threads increase.  I guess if we
wanted to do something different with -threads, it's probably best to do it
directly within Subzero, and then retune as necessary when Subzero gets more
scalable.  (Along those lines, maybe it would have been best for pnacl-llc if we
just passed it the raw number of threads reported by Chrome, and did the min()
operation to cap it within pnacl-llc instead of Chrome.)

[Jim Stichnoth, 2015-03-27 20:28:31]

Forgot one thing.  Per our offline discussion, be sure to add a "subzero" bit to
the cache key.

[Nick Bray (chromium), 2015-03-27 20:30:42]

Test stuff LGTM ish.  o_0!

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:401:
IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclSubzero,
Put this next to the original so the diff is  more obvious?

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:403:
RunLoadTest(FILE_PATH_LITERAL("pnacl_options.html?use_nmf=o_0"));
Naively, I would have expected this tests to do some sort of smoke test
postMessage "ping" to check the nexe is alive afterwards...

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.h:126: class NaClBrowserTestPnaclSubzero
: public NaClBrowserTestPnacl {
Comment about this being temporary (I assume) and why it's separate.

[jvoung (off chromium), 2015-03-27 20:46:51]

Some inline comments to hopefully make it easier to follow how the --threads=N
parameter makes its way to pnacl-sz.

Yeah, the way subzero and llc scale w/ the number of threads will be different,
so it might be good to cap it on that side, or on both sides (browser gives some
leeway and caps at 6, while pnacl-sz caps at 4 and pnacl-llc at 5) or something.

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:41:
args.push_back(MakeCommandLineArg("-split-module=", obj_files_size));
Re: threads -- LLC encodes the commandline flag for the # of threads here as a
string, as well as below.

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:60: 
Subzero does not encode the # of threads as a string.

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:66: //  
args.push_back("-mattr=" + architecture_attributes);
Otherwise, the idea is that some of argv is constructed here for subzero and
above for llc. There's only a few things that show up in argv and stuff like
-filetype is assumed to be obj (because this browser translation pipeline won't
know what to do with -filetype=asm). We also leave off cross-compilation-type
flags, etc.

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:227:
static_cast<int>(num_threads_), compile_out_files[0]->desc(),
The number of threads is also specified by the first "i" (integer) IPC
parameter.

In summary
(*) for subzero we only specify that here, and not as a string.
(*) for llc we specify it here and as string, with a TODO to not need the string
version.

Within subzero, we take this integer IPC parameter and on line 111 of
https://codereview.chromium.org/997773002/diff/540001/src/IceBrowserCompileSe...

we do:

 111   // Set some defaults which aren't specified via the argv string.
 112   Flags.setNumTranslationThreads(NumThreads);

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:235:
compile_out_files[15]->desc(), &split_args[0], split_args.size());
split_args contains that weird \0 delimited string.

The new IRT support for subzero (and later LLC), will then turn that into a
proper argv[] and argc.

[Jim Stichnoth, 2015-03-27 21:19:13]

Thanks!

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/1005173006/diff/180001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:66: //  
args.push_back("-mattr=" + architecture_attributes);
On 2015/03/27 20:46:51, jvoung wrote:
> Otherwise, the idea is that some of argv is constructed here for subzero and
> above for llc. There's only a few things that show up in argv and stuff like
> -filetype is assumed to be obj (because this browser translation pipeline
won't
> know what to do with -filetype=asm). We also leave off cross-compilation-type
> flags, etc.

If it helps, we could (and maybe should anyway) change Subzero to make
-filetype=obj the default.

[jvoung (off chromium), 2015-03-28 01:17:35]

jvoung@chromium.org changed reviewers:
+ jschuh@chromium.org

[jvoung (off chromium), 2015-03-28 01:18:34]

Plumbed a translation cache bit to distinguish between subzero and llc.

+jschuh for components/nacl/common/nacl_host_messages.h OWNERS

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:401:
IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclSubzero,
On 2015/03/27 20:30:42, Nick Bray (chromium) wrote:
> Put this next to the original so the diff is  more obvious?

Done.

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:403:
RunLoadTest(FILE_PATH_LITERAL("pnacl_options.html?use_nmf=o_0"));
On 2015/03/27 20:30:42, Nick Bray (chromium) wrote:
> Naively, I would have expected this tests to do some sort of smoke test
> postMessage "ping" to check the nexe is alive afterwards...

Good point.

I could make a separate pexe, or I could enhance the simple.cc test (though I
guess that would cascade to about 3 other test html/js).

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/1005173006/diff/180001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.h:126: class NaClBrowserTestPnaclSubzero
: public NaClBrowserTestPnacl {
On 2015/03/27 20:30:42, Nick Bray (chromium) wrote:
> Comment about this being temporary (I assume) and why it's separate.

Done.

[Jim Stichnoth, 2015-03-28 18:18:45]

lgtm

[Derek Schuff, 2015-03-30 16:39:47]

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_resources.cc (right):

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.cc:45: size_t index =
static_cast<size_t>(type);
would making ResourceType an enum class avoid the casting?

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.cc:49: DCHECK(false && "Index
out of bounds");
it looks like chrome style is
DCHECK(condition) << "message"

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_resources.h (right):

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.h:43: enum ResourceType { kLLC,
kLD, kSubzero, kNumTypes };
it looks like chromium style is all-caps for enums
http://www.chromium.org/developers/coding-style#Naming

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.h:56: PnaclResourceEntries
resources_[kNumTypes + 1];
you could make this a std::vector, which would simplify all the loops you use
over it (range-for, etc)

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:68: for
(Args::const_iterator arg(args.begin()); arg != args.end(); ++arg) {
range-for?

[jvoung (off chromium), 2015-03-30 20:26:40]

Thanks

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_resources.cc (right):

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.cc:45: size_t index =
static_cast<size_t>(type);
On 2015/03/30 16:39:47, Derek Schuff wrote:
> would making ResourceType an enum class avoid the casting?

Doesn't seem to help:

http://stackoverflow.com/questions/12927951/array-indexing-converting-to-inte...

"no known conversion from 'plugin::PnaclResources::ResourceType' to 'size_type'"

and I'm not sure if "std::underlying_type" for converting is considered a
"library feature"... c++11 library features aren't allowed yet.

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.cc:49: DCHECK(false && "Index
out of bounds");
On 2015/03/30 16:39:47, Derek Schuff wrote:
> it looks like chrome style is
> DCHECK(condition) << "message"

=(

Unfortunately this is the NaCl DCHECK macro. I tried using base/logging.h but
stuff like "LOG_FATAL" is a macro for NaCl (defined as -4) and conflicts with
LOG_FATAL (not a macro, defined as 3) in base/logging.h.

Added a TODO to use cleaner checks.

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_resources.h (right):

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.h:43: enum ResourceType { kLLC,
kLD, kSubzero, kNumTypes };
On 2015/03/30 16:39:47, Derek Schuff wrote:
> it looks like chromium style is all-caps for enums
> http://www.chromium.org/developers/coding-style#Naming

DONE

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.h:56: PnaclResourceEntries
resources_[kNumTypes + 1];
On 2015/03/30 16:39:47, Derek Schuff wrote:
> you could make this a std::vector, which would simplify all the loops you use
> over it (range-for, etc)

I think plain arrays also support range-for, but could convert to std::vector if
you prefer.

Changed to range-for.

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:68: for
(Args::const_iterator arg(args.begin()); arg != args.end(); ++arg) {
On 2015/03/30 16:39:47, Derek Schuff wrote:
> range-for?

Done.

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:227:
static_cast<int>(num_threads_), compile_out_files[0]->desc(),
Reverted this back to an "int" field to avoid the cast.

[Derek Schuff, 2015-03-30 21:03:12]

pnacl coordinator and translation cache LGTM

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
File components/nacl/renderer/plugin/pnacl_resources.cc (right):

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.cc:45: size_t index =
static_cast<size_t>(type);
On 2015/03/30 20:26:40, jvoung wrote:
> On 2015/03/30 16:39:47, Derek Schuff wrote:
> > would making ResourceType an enum class avoid the casting?
> 
> Doesn't seem to help:
> 
>
http://stackoverflow.com/questions/12927951/array-indexing-converting-to-inte...
> 
> "no known conversion from 'plugin::PnaclResources::ResourceType' to
'size_type'"
> 
> and I'm not sure if "std::underlying_type" for converting is considered a
> "library feature"... c++11 library features aren't allowed yet.

yeah it's declared in <type_traits>. oh well.

https://codereview.chromium.org/1005173006/diff/220001/components/nacl/render...
components/nacl/renderer/plugin/pnacl_resources.cc:49: DCHECK(false && "Index
out of bounds");
On 2015/03/30 20:26:40, jvoung wrote:
> On 2015/03/30 16:39:47, Derek Schuff wrote:
> > it looks like chrome style is
> > DCHECK(condition) << "message"
> 
> =(
> 
> Unfortunately this is the NaCl DCHECK macro. I tried using base/logging.h but
> stuff like "LOG_FATAL" is a macro for NaCl (defined as -4) and conflicts with
> LOG_FATAL (not a macro, defined as 3) in base/logging.h.
> 
> Added a TODO to use cleaner checks.

fun :/

[jvoung (off chromium), 2015-03-31 14:57:15]

jvoung@chromium.org changed reviewers:
+ tsepez@chromium.org

[jvoung (off chromium), 2015-03-31 14:58:50]

or Tom for components/nacl/common/nacl_host_messages.h OWNERS

[Tom Sepez, 2015-03-31 16:26:51]

Messages LGTM.

[jvoung (off chromium), 2015-03-31 16:54:30]

The CQ bit was checked by jvoung@chromium.org

[jvoung (off chromium), 2015-03-31 16:54:30]

The patchset sent to the CQ was uploaded after l-g-t-m from ncbray@chromium.org,
stichnot@chromium.org, dschuff@chromium.org
Link to the patchset: https://codereview.chromium.org/1005173006/#ps280001
(title: "fix takefileinfo")

[commit-bot: I haz the power, 2015-03-31 16:54:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1005173006/280001

[commit-bot: I haz the power, 2015-03-31 17:19:46]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-03-31 17:20:52]

Patchset 15 (id:??) landed as
https://crrev.com/58bea9692855603927912636d029586a90d60452
Cr-Commit-Position: refs/heads/master@{#323063}