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

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

Some pnacl-translate arguments are unsuitable for Subzero, such as:
  -sfi-*
  --allow-llvm-bitcode-input
  -fPIC
  -arch specifying non-x86-32
This triggers a fallback to pnacl-llc.  No special warnings are issued on a fallback, but one can run with --pnacl-driver-verbose and look for pnacl-llc in the output as a fallback indication.

This has been tested manually by setting default USE_SZ to 1 and running the LLVM test suite and the gcc torture tests.  (Also tested on the scons smoke_tests, revealing one unresolved bug at this writing.)  A follow-on CL will enable Subzero tests on the bots.

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

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/bac8dff0bcf40b76d96576880f68d63c173501d1

[Jim Stichnoth, 2015-03-21 03:47:05]

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

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

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
File pnacl/driver/pnacl-translate.py (right):

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:150: 'SZ_THREADS' : '0',
Can you use SPLIT_MODULE directly?

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:175: "env.append('SZ_FLAGS_EXTRA', $0)"),
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.

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:215: "env.append('SZ_FLAGS_EXTRA',
'--build-on-read=0')"),
I don't understand why add SZ flags when it's unsupported.

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

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
File pnacl/driver/pnacl-translate.py (right):

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:146: 'USE_SZ' : '1',
Oops this needs to default to '0'

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:150: 'SZ_THREADS' : '0',
On 2015/03/21 04:41:27, JF wrote:
> Can you use SPLIT_MODULE directly?

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.

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:175: "env.append('SZ_FLAGS_EXTRA', $0)"),
On 2015/03/21 04:41:27, JF wrote:
> 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.

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.

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:215: "env.append('SZ_FLAGS_EXTRA',
'--build-on-read=0')"),
On 2015/03/21 04:41:27, JF wrote:
> I don't understand why add SZ flags when it's unsupported.

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

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

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
File pnacl/driver/pnacl-translate.py (right):

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:175: "env.append('SZ_FLAGS_EXTRA', $0)"),
On 2015/03/21 04:41:27, JF wrote:
> 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.

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)

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:181: "env.append('SZ_FLAGS_EXTRA', $0)"),
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.

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:302: env.set('SZ_UNSUPPORTED', '1')
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.

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:592: --pnacl-sb              Use the translator
which runs inside the NaCl sandbox.
Add --sz to the help message, if you want to advertise it a tiny bit more.

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

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
File pnacl/driver/pnacl-translate.py (right):

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:175: "env.append('SZ_FLAGS_EXTRA', $0)"),
On 2015/03/21 18:06:29, jvoung wrote:
> On 2015/03/21 04:41:27, JF wrote:
> > 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.
> 
> 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)

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

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:181: "env.append('SZ_FLAGS_EXTRA', $0)"),
On 2015/03/21 18:06:30, jvoung wrote:
> 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.

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

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:302: env.set('SZ_UNSUPPORTED', '1')
On 2015/03/21 18:06:29, jvoung wrote:
> 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.

Sounds good, done.

https://codereview.chromium.org/1022143004/diff/1/pnacl/driver/pnacl-translat...
pnacl/driver/pnacl-translate.py:592: --pnacl-sb              Use the translator
which runs inside the NaCl sandbox.
On 2015/03/21 18:06:30, jvoung wrote:
> Add --sz to the help message, if you want to advertise it a tiny bit more.

Done.

[JF, 2015-03-23 16:29:05]

lgtm

[jvoung (off chromium), 2015-03-23 16:49:26]

lgtm

[Derek Schuff, 2015-03-23 16:54:47]

lgtm

[Jim Stichnoth, 2015-03-23 20:58:46]

Committed patchset #3 (id:40001) manually as
bac8dff0bcf40b76d96576880f68d63c173501d1 (presubmit successful).

[Mark Seaborn, 2015-03-23 21:39:56]

Drive-by nit: Using the name "--sz" is obscure.  It looks like an abbreviation
for "size".  How about naming this "--subzero" instead?

"Subzero" is already somewhat obscure -- or at least not self-describing -- as a
project name, so having two names for it ("subzero" and "sz") makes it harder
for someone who's new to this to figure out what's going on.

[Jim Stichnoth, 2015-03-24 03:22:30]

On 2015/03/23 21:39:56, Mark Seaborn wrote:
> Drive-by nit: Using the name "--sz" is obscure.  It looks like an abbreviation
> for "size".  How about naming this "--subzero" instead?
> 
> "Subzero" is already somewhat obscure -- or at least not self-describing -- as
a
> project name, so having two names for it ("subzero" and "sz") makes it harder
> for someone who's new to this to figure out what's going on.

I chose --sz to reflect the tool name pnacl-sz, but you're right that it's a bit
obscure.

Maybe --use-sz?  This would be in line with the use-sz and use_sz controls that
I'm currently adding to the scons tests, LLVM test suite, and gcc torture tests.

Also, this made me realize that pnacl-translate.py has help text near the end
where I should have described this flag.  I'll take care of all of this in the
next CL.

[native-client-reviews_googlegroups.com, 2015-03-24 03:35:50]

On 23 March 2015 at 20:22, <stichnot@chromium.org> wrote:

> On 2015/03/23 21:39:56, Mark Seaborn wrote:
>
>> Drive-by nit: Using the name "--sz" is obscure.  It looks like an
>> abbreviation
>> for "size".  How about naming this "--subzero" instead?
>>
>
>  "Subzero" is already somewhat obscure -- or at least not self-describing
>> -- as a
>
>  project name, so having two names for it ("subzero" and "sz") makes it
>> harder
>> for someone who's new to this to figure out what's going on.
>>
>
> I chose --sz to reflect the tool name pnacl-sz, but you're right that it's
> a bit
> obscure.
>

Yeah, I noticed changes mentioning "pnacl-sz" go by, and I thought that was
a bit of an obscure name too.  It still makes me think "pnacl-size". :-)

Cheers,
Mark

-- 
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.