Closed
Bug 1286725
Opened 5 years ago
Closed 5 years ago
Clean up the jit_test jitflags handling
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(2 files)
5.93 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
--ion and --tbpl are really just variants of --jitflags.
Attachment #8770776 -
Flags: review?(terrence)
Assignee | ||
Comment 2•5 years ago
|
||
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 3•5 years ago
|
||
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 4•5 years ago
|
||
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
Comment 6•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/89192956a41a https://hg.mozilla.org/mozilla-central/rev/09a1881a4f7e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•