Add pnacl-translate --sz flag to enable Subzero.

pnacl/driver/pnacl-translate.py

Index: pnacl/driver/pnacl-translate.py
diff --git a/pnacl/driver/pnacl-translate.py b/pnacl/driver/pnacl-translate.py
index 60e2962c13a372e75be1a8b7217e590f640b4c7c..fe33c3c5844ded836eae90559f17defe810dc54a 100755
--- a/pnacl/driver/pnacl-translate.py
+++ b/pnacl/driver/pnacl-translate.py
@@ -109,6 +109,7 @@ EXTRA_ENV = {
   'LLC_FLAGS_EXTRA' : '${FAST_TRANSLATION ? ${LLC_FLAGS_FAST}} ' +
                       '${#OPT_LEVEL ? -O${OPT_LEVEL}} ' +
                       '${OPT_LEVEL == 0 ? -disable-fp-elim}',
+  'SZ_FLAGS_EXTRA' : '--filetype=${outfiletype}',
 
   # Opt level from command line (if any)
   'OPT_LEVEL' : '',
@@ -124,6 +125,8 @@ EXTRA_ENV = {
   # Note: this is only used in the unsandboxed case
   'RUN_LLC'       : '${LLVM_PNACL_LLC} ${LLC_FLAGS} ${LLC_MCPU} '
                     '${input} -o ${output} ',
+  'RUN_SZ': '${LLVM_PNACL_SZ} ${SZ_FLAGS_ARCH} ${SZ_FLAGS_EXTRA} '
+            '${input} -o ${output}',
   # Whether to stream the bitcode from a single FD in unsandboxed mode
   # (otherwise it will use concurrent file reads when using multithreaded module
   # splitting)
@@ -139,6 +142,12 @@ EXTRA_ENV = {
   # with faster compilation, whereas 'static' will still use multiple cores but
   # will be deterministic and slightly slower.
   'SPLIT_MODULE_SCHED' : '${SANDBOXED ? dynamic : static}',
+  # Whether to (try to) use pnacl-sz for translation instead of pnacl-llc.
+  'USE_SZ' : '1',

[Jim Stichnoth, 2015/03/21 17:33:19]

Oops this needs to default to '0'

+  # Whether an option has been specified that Subzero can't (yet) handle.
+  'SZ_UNSUPPORTED' : '0',
+  # Subzero equivalent of SPLIT_MODULE, i.e. default # of translation threads.
+  'SZ_THREADS' : '0',

[JF, 2015/03/21 04:41:27]

Can you use SPLIT_MODULE directly?

[Jim Stichnoth, 2015/03/21 17:33:19]

The problem is that SPLIT_MODULE is used for several things that aren't
necessarily compatible with Subzero:
  - Specify number of translation threads (this is fine)
  - Construct additional input filenames for the linker
  - Disable SPLIT_MODULE in situations that aren't necessarily relevant to
Subzero
  - SPLIT_MODULE is required to be >= 1 whereas pnacl-sz --threads=0 and
--threads=1 are distinctly useful

Since SPLIT_MODULE is so overloaded, it seemed cleaner to have a separate,
simpler mechanism for controlling Subzero threading.

 }
 
 
@@ -156,16 +165,20 @@ TranslatorPatterns = [
   # the pnacl-translate commandline tool and the in-browser translator.
   # See: llvm/tools/pnacl-llc/srpc_main.cpp and
   # Chromium's plugin/pnacl_translate_thread.cc
-  ( '(-sfi-.+)',        "env.append('LLC_FLAGS_EXTRA', $0)"),
+  ( '(-sfi-.+)',        "env.append('LLC_FLAGS_EXTRA', $0)\n"
+                        "env.set('SZ_UNSUPPORTED', '1')"),
   ( '(-mtls-use-call)', "env.append('LLC_FLAGS_EXTRA', $0)"),
   ( '(-force-align-stack)', "env.append('LLC_FLAGS_EXTRA', $0)"),
   # These flags are usually used for linktime dead code/data
   # removal but also help with reloc overflows on ARM
-  ( '(-fdata-sections)',     "env.append('LLC_FLAGS_EXTRA', '-data-sections')"),
+  ( '(-fdata-sections)',     "env.append('LLC_FLAGS_EXTRA', '-data-sections')\n"
+                             "env.append('SZ_FLAGS_EXTRA', $0)"),

[JF, 2015/03/21 04:41:27]

Use LLC_FLAGS_EXTRA and teach subzero to accept the same arguments as the
translator? It looks like some flags like -force-align-stack get dropped
otherwise.

[Jim Stichnoth, 2015/03/21 17:33:19]

Well, it's more than LLC_FLAGS_EXTRA.  There's LLC_FLAGS_TARGET,
LLC_FLAGS_COMMON, LLC_FLAGS_ARCH, LLC_MCPU, plus a couple more that dribble in
up to and including the RunLLC call.

That's kind of minor.  My actual revulsion kicks in at the thought of
deconstructing individual arguments from a command line that might contain
quoted/escaped spaces, shell wildcards, etc.  Also having to normalize things
like "--arg foo" vs "--arg=foo".  For those reasons I preferred to capture
arguments from the start and construct separate command lines.

Also, there is the escape hatch where you can call these driver scripts with
arguments like --pnacl-driver-set-LLC_FLAGS_EXTRA=-disable-fp-elim or similar
with --pnacl-driver-append.  That doesn't really kill either approach (by which
I mean this patchset versus the suggestion of command line
deconstruction/translation), but it's one more piece of the puzzle.

Good point about -force-align-stack.  I misunderstood what it does in llc.  I
changed it now to trigger SZ_UNSUPPORTED=1.

[jvoung (off chromium), 2015/03/21 18:06:29]

Or -force-align-stack should also cause SZ_UNSUPPORTED to be set. That was added
for the x86-32 IRT build w/ PNaCl, but the x86-32 IRT build is moving to
nacl-clang, so we might even be able to remove this from pnacl-translate.

(okay see you just added SZ_UNSUPPORTED)

[Jim Stichnoth, 2015/03/22 04:20:42]

We could also implement -force-align-stack in Subzero, but it sounds like that
need may go away?

   ( '(-ffunction-sections)',
-    "env.append('LLC_FLAGS_EXTRA', '-function-sections')"),
+    "env.append('LLC_FLAGS_EXTRA', '-function-sections')\n"
+    "env.append('SZ_FLAGS_EXTRA', $0)"),
   ( '(--gc-sections)',       "env.append('LD_FLAGS', $0)"),
-  ( '(-mattr=.*)', "env.append('LLC_FLAGS_EXTRA', $0)"),
+  ( '(-mattr=.*)', "env.append('LLC_FLAGS_EXTRA', $0)\n"
+                   "env.append('SZ_FLAGS_EXTRA', $0)"),

[jvoung (off chromium), 2015/03/21 18:06:30]

The way mattr works is a bit different between LLC and SZ, so something to watch
out for. LLC uses a comma separated-list w/ plusses
"-mattr=+sse4.1,+ssse3,+sse2", while SZ just takes one "-mattr=sse4.1".

There was some issue with some bot VMs not supporting SSSE3:
https://codereview.chromium.org/337243002, so that might explain why no scons
test actually uses -mattr. Ideally the bots would support it so that we could
actually test SSSE3 and SSE4.1.

[Jim Stichnoth, 2015/03/22 04:20:43]

Ah, thanks.  I added a Subzero issue for that.
https://code.google.com/p/nativeclient/issues/detail?id=4132

   ( '(-mcpu=.*)', "env.set('LLC_MCPU', '')\n"
                   "env.append('LLC_FLAGS_EXTRA', $0)"),
   ( '(-pnaclabi-verify=.*)', "env.append('LLC_FLAGS_EXTRA', $0)"),
@@ -176,6 +189,8 @@ TranslatorPatterns = [
   # This adds arch specific flags to the llc invocation aimed at
   # improving translation speed at the expense of code quality.
   ( '-translate-fast',  "env.set('FAST_TRANSLATION', '1')"),
+  # Allow Subzero.
+  ( '--sz', "env.set('USE_SZ', '1')"),
 
   ( '-nostdlib',       "env.set('USE_STDLIB', '0')"),
 
@@ -193,13 +208,19 @@ TranslatorPatterns = [
   # "--pnacl-allow-zerocost-eh".
   ( '--pnacl-allow-exceptions', "env.set('ALLOW_ZEROCOST_CXX_EH', '1')"),
 
-  ( '--allow-llvm-bitcode-input', "env.set('ALLOW_LLVM_BITCODE_INPUT', '1')"),
+  ( '--allow-llvm-bitcode-input',
+    "env.set('ALLOW_LLVM_BITCODE_INPUT', '1')\n"
+    "env.set('SZ_UNSUPPORTED', '1')\n"
+    "env.append('SZ_FLAGS_EXTRA', '--bitcode-format=llvm')\n"
+    "env.append('SZ_FLAGS_EXTRA', '--build-on-read=0')"),

[JF, 2015/03/21 04:41:27]

I don't understand why add SZ flags when it's unsupported.

[Jim Stichnoth, 2015/03/21 17:33:19]

Whoops, that was left over from a trip down the --allow-llvm-bitcode-input
rabbit hole.

 
-  ( '-fPIC',           "env.set('PIC', '1')"),
+  ( '-fPIC',           "env.set('PIC', '1')\n"
+                       "env.set('SZ_UNSUPPORTED', '1')"),
 
   ( '(--build-id)',    "env.append('LD_FLAGS', $0)"),
   ( '-bitcode-stream-rate=([0-9]+)', "env.set('BITCODE_STREAM_RATE', $0)"),
-  ( '-split-module=([0-9]+)', "env.set('SPLIT_MODULE', $0)"),
+  ( '-split-module=([0-9]+)', "env.set('SPLIT_MODULE', $0)\n"
+                              "env.set('SZ_THREADS', $0)"),
   ( '-split-module-sched=(.*)', "env.set('SPLIT_MODULE_SCHED', $0)"),
   ( '-no-stream-bitcode', "env.set('STREAM_BITCODE', '0')"),
 
@@ -266,12 +287,29 @@ def SetUpArch():
       'X8632_NONSFI': ['-malign-double'],
       }
   env.set('LLC_FLAGS_ARCH', *llc_flags_map.get(env.getone('ARCH'), []))
+  env.set('SZ_FLAGS_ARCH', '')
   # When linking against a host OS's libc (such as Linux glibc), don't
   # use %gs:0 to read the thread pointer because that won't be
   # compatible with the libc's use of %gs:0.  Similarly, Non-SFI Mode
   # currently offers no optimized path for reading the thread pointer.
   if env.getone('TARGET_OS') != 'nacl' or env.getbool('NONSFI_NACL'):
     env.append('LLC_FLAGS_ARCH', '-mtls-use-call')
+  # For Subzero, determine -target and -sandbox options.
+  env.append('SZ_FLAGS_ARCH', '--sandbox=' +
+             ('1' if env.getone('TARGET_OS') == 'nacl' else '0'))
+  env.append('SZ_FLAGS_ARCH', '--target=' + base_arch.lower())
+  if base_arch != 'X8632':
+    env.set('SZ_UNSUPPORTED', '1')

[jvoung (off chromium), 2015/03/21 18:06:29]

The fallbacks are nice when, within the supported arches, a test is doing
something unsupported.

However, for completely unsupported arches it might be good to just report a
fatal error (otherwise it's a 100% fallback)? I imagine the bots will be set up
to only test supported arches.

[Jim Stichnoth, 2015/03/22 04:20:43]

Sounds good, done.

+  # This is a fine place to map OPT_LEVEL to the Subzero equivalent, with
+  # default of -O2.
+  sz_opt_map = {
+    '0': '-Om1',
+    '1': '-O2',
+    '2': '-O2',
+    }
+  env.append('SZ_FLAGS_ARCH', sz_opt_map.get(env.getone('OPT_LEVEL'), '-O2'))
+  # At this point, the only Subzero options left to set are -o, -filetype, and
+  # -threads.
 
 
 def SetUpLinkOptions():
@@ -303,6 +341,9 @@ def main(argv):
   SetUpArch()
   SetUpLinkOptions()
 
+  # Now commit to whether or not Subzero is used.
+  use_sz = env.getbool('USE_SZ') and not env.getbool('SZ_UNSUPPORTED')
+
   inputs = env.get('INPUTS')
   output = env.getone('OUTPUT')
 
@@ -355,9 +396,11 @@ def main(argv):
       env.append('LLC_FLAGS_EXTRA', '-streaming-bitcode')
   modules = env.getone('SPLIT_MODULE')
   module_sched = env.getone('SPLIT_MODULE_SCHED')
+  sz_threads = env.getone('SZ_THREADS')
   env.append('LLC_FLAGS_EXTRA', '-split-module=' + modules)
-  env.append('LD_FLAGS', '-split-module=' + modules)
+  env.append('LD_FLAGS', '-split-module=' + ('1' if use_sz else modules))
   env.append('LLC_FLAGS_EXTRA', '-split-module-sched=' + module_sched)
+  env.append('SZ_FLAGS_EXTRA', '--threads=' + sz_threads)
 
   # If there's a bitcode file, translate it now.
   tng = driver_tools.TempNameGen(inputs + bcfiles, output)
@@ -374,11 +417,11 @@ def main(argv):
       ofile = tng.TempNameForInput(bcfile, 'o')
 
     if sfile:
-      RunLLC(bcfile, sfile, outfiletype='asm')
+      RunLLC(bcfile, sfile, outfiletype='asm', use_sz=use_sz)
       if ofile:
         RunAS(sfile, ofile)
     else:
-      RunLLC(bcfile, ofile, outfiletype='obj')
+      RunLLC(bcfile, ofile, outfiletype='obj', use_sz=use_sz)
   else:
     ofile = None
 
@@ -386,6 +429,10 @@ def main(argv):
   if output_type in ('o','s'):
     return 0
 
+  if use_sz:
+    # Reset SPLIT_MODULE to 1 to fall back to normal linking behavior.
+    env.set('SPLIT_MODULE', '1')
+
   # Replace the bitcode file with __BITCODE__ in the input list
   if bcfile:
     inputs = ListReplace(inputs, bcfile, '__BITCODE__')
@@ -446,14 +493,14 @@ def RunHostLD(infile, outfile):
     args.append('-lrt')  # For clock_gettime()
   driver_tools.Run(args)
 
-def RunLLC(infile, outfile, outfiletype):
+def RunLLC(infile, outfile, outfiletype, use_sz):
   env.push()
   env.setmany(input=infile, output=outfile, outfiletype=outfiletype)
   if env.getbool('SANDBOXED'):
     RunLLCSandboxed()
     env.pop()
   else:
-    args = ["${RUN_LLC}"]
+    args = ["${RUN_SZ}" if use_sz else "${RUN_LLC}"]
     if filetype.IsPNaClBitcode(infile):
       args.append("-bitcode-format=pnacl")
     elif filetype.IsLLVMBitcode(infile):