1286725 - Clean up the jit_test jitflags handling

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

When working on bug 1280637, I needed to add --jitflags=interp, and found that the existing jit_test handling of that option isn't quite right. Some fixes.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Clean up jitflags

--ion and --tbpl are really just variants of --jitflags.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Do not add wasm-baseline variants when interpreter-only is requested

I can't say I'm fond of this patch. But the current setup is broken, yet used, so I need to fix it for --jitflags=interp.

This patch makes it so --jitflags=interp will not run the wasm baseline jit, as that would kind of defeat the point of being interpreter-only.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8770776 [details] [diff] [review]
Clean up jitflags

Review of attachment 8770776 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8770779 [details] [diff] [review]
Do not add wasm-baseline variants when interpreter-only is requested

Review of attachment 8770779 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/jit_test.py
@@ +203,5 @@
> +    # contains test-also-noasmjs.)
> +    test_flags = get_jitflags(options.jitflags)
> +    if all(['--no-asmjs' in flags for flags in test_flags]):
> +        options.can_test_also_noasmjs = False
> +        options.can_test_also_wasm_baseline = False

How about we combine the two "is asmjs allowed" checks like so?

# "Combined comment explaining when it's not safe to run asmjs during testing."
allow_asmjs = not options.debugger and \
              not all(['--no-asmjs' in flags for flags in test_flags])
if not allow_asmjs:
    options.can_test_also_noasmjs = False
    options.can_test_also_wasm_baseline = False

Actually, that's a triple negative and introduces a new variable in the top scope. So this could actually be:

# "Comment..."
if options.debugger or all(['--no-asmjs' in flags for flags in test_flags]):
    options.can_test_also_noasmjs = False
    options.can_test_also_wasm_baseline = False

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89192956a41a
Clean up jitflags, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/09a1881a4f7e
Do not add wasm-baseline variants when interpreter-only is requested, r=terrence

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89192956a41a
https://hg.mozilla.org/mozilla-central/rev/09a1881a4f7e