[DFG][FTL] regExpMatchFast should be handled
Created attachment 329795 [details] Patch
Looks like you upset the EWS. :(
Comment on attachment 329795 [details] Patch Attachment 329795 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5757662 New failing tests: css3/shapes/shape-outside/values/shape-outside-ellipse-004.html css3/shapes/shape-outside/values/shape-outside-circle-004.html imported/w3c/web-platform-tests/css/css-shapes-1/shape-outside/values/shape-outside-ellipse-004.html
Created attachment 329808 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329795 [details] Patch Attachment 329795 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/5757696 New failing tests: mozilla-tests.yaml/ecma_2/String/match-001.js.mozilla-dfg-eager-no-cjit-validate-phases stress/regress-174044.js.ftl-no-cjit-small-pool stress/regress-174044.js.default stress/regress-174044.js.ftl-eager-no-cjit-b3o1 mozilla-tests.yaml/ecma_2/String/match-003.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma_2/String/match-001.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma_2/String/match-003.js.mozilla-dfg-eager-no-cjit-validate-phases stress/regress-174044.js.dfg-eager-no-cjit-validate stress/regress-174044.js.ftl-no-cjit-no-put-stack-validate stress/regress-174044.js.dfg-eager stress/regress-174044.js.no-cjit-collect-continuously stress/regress-174044.js.no-ftl stress/regress-174044.js.ftl-no-cjit-b3o1 stress/regress-174044.js.no-llint stress/regress-174044.js.ftl-eager-no-cjit stress/regress-174044.js.ftl-no-cjit-validate-sampling-profiler stress/regress-174044.js.ftl-eager stress/regress-174044.js.no-cjit-validate-phases stress/regress-174044.js.dfg-maximal-flush-validate-no-cjit
Comment on attachment 329795 [details] Patch Attachment 329795 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5757733 New failing tests: css3/shapes/shape-outside/values/shape-outside-ellipse-004.html css3/shapes/shape-outside/values/shape-outside-circle-004.html svg/custom/object-sizing-explicit-width.xhtml imported/w3c/web-platform-tests/css/css-shapes-1/shape-outside/values/shape-outside-ellipse-004.html
Created attachment 329819 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329795 [details] Patch Attachment 329795 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5758585 New failing tests: css3/shapes/shape-outside/values/shape-outside-ellipse-004.html css3/shapes/shape-outside/values/shape-outside-circle-004.html imported/w3c/web-platform-tests/css/css-shapes-1/shape-outside/values/shape-outside-ellipse-004.html
Created attachment 329828 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 329986 [details] Patch
Created attachment 329987 [details] Patch
Created attachment 329988 [details] Patch
Ping?
Comment on attachment 329988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329988&action=review Neat! > Source/JavaScriptCore/builtins/RegExpPrototype.js:85 > + 'use strict'; Style: Most of the time this is a double quoted string.
Comment on attachment 329988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329988&action=review >> Source/JavaScriptCore/builtins/RegExpPrototype.js:85 >> + 'use strict'; > > Style: Most of the time this is a double quoted string. Thanks, fixed.
Created attachment 330509 [details] Patch
Created attachment 330510 [details] Patch
Comment on attachment 330510 [details] Patch Attachment 330510 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5934791 New failing tests: fast/mediastream/MediaStream-MediaElement-setObject-null.html
Created attachment 330516 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
(In reply to Build Bot from comment #19) > Created attachment 330516 [details] > Archive of layout-test-results from ews106 for mac-elcapitan-wk2 > > The attached test failures were seen while running run-webkit-tests on the > mac-wk2-ews. > Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6 Seems unrelated.
Comment on attachment 330510 [details] Patch r=me
(In reply to Yusuke Suzuki from comment #20) > (In reply to Build Bot from comment #19) > > Created attachment 330516 [details] > > Archive of layout-test-results from ews106 for mac-elcapitan-wk2 > > > > The attached test failures were seen while running run-webkit-tests on the > > mac-wk2-ews. > > Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6 > > Seems unrelated. Can you please run the failed test locally and confirm that it passes with your patch before landing?
Comment on attachment 330510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330510&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1968 > + case RegExpMatch: This name confused me. I vote for calling this RegExpMatchFast to align w/ the intrinsic. I originally thought this was the entire match operation, but it's only the fast path.
Comment on attachment 330510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330510&action=review LGTM too > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2621 > + if (argumentCountIncludingThis != 2) > + return false; Seems like this should be a release assert since this is only called by our builtins. > Source/JavaScriptCore/dfg/DFGOperations.cpp:1026 > +EncodedJSValue JIT_OPERATION operationRegExpMatchString(ExecState* exec, JSGlobalObject* globalObject, RegExpObject* regExpObject, JSString* argument) I would also add "fast" here to align the names. > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:-149 > - VM& vm = exec->vm(); > - auto scope = DECLARE_THROW_SCOPE(vm); > - > - JSValue thisValue = exec->thisValue(); > - if (!thisValue.inherits(vm, RegExpObject::info())) > - return throwVMTypeError(exec, scope); > - JSString* string = exec->argument(0).toStringOrNull(exec); > - EXCEPTION_ASSERT(!!scope.exception() == !string); > - if (!string) > - return encodedJSValue(); > - if (!asRegExpObject(thisValue)->regExp()->global()) { > - scope.release(); > - return JSValue::encode(asRegExpObject(thisValue)->exec(exec, exec->lexicalGlobalObject(), string)); > - } > - scope.release(); > - return JSValue::encode(asRegExpObject(thisValue)->matchGlobal(exec, exec->lexicalGlobalObject(), string)); This was all unneeded because we already do the safety checks in the builtin, right? Why was this here before then?
Comment on attachment 330510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330510&action=review Thanks! >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1968 >> + case RegExpMatch: > > This name confused me. I vote for calling this RegExpMatchFast to align w/ the intrinsic. I originally thought this was the entire match operation, but it's only the fast path. OK, changed it to RegExpMatchFast. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2621 >> + return false; > > Seems like this should be a release assert since this is only called by our builtins. Right. Changed. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:1026 >> +EncodedJSValue JIT_OPERATION operationRegExpMatchString(ExecState* exec, JSGlobalObject* globalObject, RegExpObject* regExpObject, JSString* argument) > > I would also add "fast" here to align the names. Fixed. >> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:-149 >> - return JSValue::encode(asRegExpObject(thisValue)->matchGlobal(exec, exec->lexicalGlobalObject(), string)); > > This was all unneeded because we already do the safety checks in the builtin, right? Why was this here before then? Right. This function is only called if the above check is met. So this is unnecessary. I'm not sure why we have these unnecessary checks.
(In reply to Mark Lam from comment #23) > (In reply to Yusuke Suzuki from comment #20) > > (In reply to Build Bot from comment #19) > > > Created attachment 330516 [details] > > > Archive of layout-test-results from ews106 for mac-elcapitan-wk2 > > > > > > The attached test failures were seen while running run-webkit-tests on the > > > mac-wk2-ews. > > > Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6 > > > > Seems unrelated. > > Can you please run the failed test locally and confirm that it passes with > your patch before landing? OK, the test runs as expected in my working copy.
Committed r226775: <https://trac.webkit.org/changeset/226775>
<rdar://problem/36436985>