Closed Bug 1286725 Opened 3 years ago Closed 3 years ago

Clean up the jit_test jitflags handling

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(2 files)

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.
--ion and --tbpl are really just variants of --jitflags.
Attachment #8770776 - Flags: review?(terrence)
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.
Attachment #8770779 - Flags: review?(terrence)
Comment on attachment 8770776 [details] [diff] [review]
Clean up jitflags

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

Nice!
Attachment #8770776 - Flags: review?(terrence) → review+
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
Attachment #8770779 - Flags: review?(terrence) → review+
Pushed by sfink@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/89192956a41a
https://hg.mozilla.org/mozilla-central/rev/09a1881a4f7e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1288716
You need to log in before you can comment on or make changes to this bug.