Summary: | B3StackmapSpecial should handle when stackmap values are not recoverable from a Def'ed arg. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, fpizlo, jfbastien, keith_miller, msaboff, rniwa, saam, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Mark Lam
2017-04-18 18:03:19 PDT
Created attachment 307446 [details]
proposed patch.
Comment on attachment 307446 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=307446&action=review Have you measured perf? r=me with suggested changes and perf results. Octane is particularly sensitive to CheckSpecial not using late unless it really has to. If my guess is right, this will be neutral on benchmarks - but that needs to be verified. > Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:65 > + auto argWidth = inst.origin->resultWidth(); > + optionalDefArgWidth = argWidth; > + callback(inst.args[argIndex++], role, inst.origin->resultBank(), argWidth); > } > > - forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback); > + forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback, optionalDefArgWidth); I think that in Patchpoint, you don't need to do anything like this. Patchpoint does not have Check's weird recover-value-on-overflow behavior, since it has no notion of overflow. You should pass nullopt here. > Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:129 > + // If the Def'ed arg has a smaller width than the a stackmap value, then we may not > + // be able to recover the stackmap value. So, force LateColdUse to preserve the > + // original stackmap value across the Special operation. > + if (optionalDefArgWidth && *optionalDefArgWidth < child.value()->resultWidth()) > + role = Arg::LateColdUse; I think that you want it to be a LateUse if it was a Use and a LateColdUse if it was a ColdUse. If it's already some kind of late use then you don't need to change it. Comment on attachment 307446 [details] proposed patch. Attachment 307446 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3560267 New failing tests: wasm.yaml/wasm/function-tests/many-arguments-to-function.js.default-wasm wasm.yaml/wasm/js-api/wasm-to-wasm.js.default-wasm wasm.yaml/wasm/spec-tests/left-to-right.wast.js.default-wasm Comment on attachment 307446 [details] proposed patch. Attachment 307446 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3560351 New failing tests: workers/wasm-hashset-many-2.html workers/wasm-hashset-many.html workers/wasm-hashset.html workers/wasm-long-compile-many.html Created attachment 307451 [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 307446 [details] proposed patch. Attachment 307446 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3560415 New failing tests: workers/wasm-hashset-many-2.html workers/wasm-hashset-many.html workers/wasm-hashset.html Created attachment 307453 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307446 [details] proposed patch. Attachment 307446 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3560428 New failing tests: workers/wasm-hashset-many-2.html workers/wasm-hashset-many.html workers/wasm-hashset.html Created attachment 307456 [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 Filip Pizlo from comment #2) > Comment on attachment 307446 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307446&action=review > > Have you measured perf? Checking now. > r=me with suggested changes and perf results. Octane is particularly > sensitive to CheckSpecial not using late unless it really has to. If my > guess is right, this will be neutral on benchmarks - but that needs to be > verified. > > > Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:65 > > + auto argWidth = inst.origin->resultWidth(); > > + optionalDefArgWidth = argWidth; > > + callback(inst.args[argIndex++], role, inst.origin->resultBank(), argWidth); > > } > > > > - forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback); > > + forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback, optionalDefArgWidth); > > I think that in Patchpoint, you don't need to do anything like this. > Patchpoint does not have Check's weird recover-value-on-overflow behavior, > since it has no notion of overflow. You should pass nullopt here. Fixed. > > Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:129 > > + // If the Def'ed arg has a smaller width than the a stackmap value, then we may not > > + // be able to recover the stackmap value. So, force LateColdUse to preserve the > > + // original stackmap value across the Special operation. > > + if (optionalDefArgWidth && *optionalDefArgWidth < child.value()->resultWidth()) > > + role = Arg::LateColdUse; > > I think that you want it to be a LateUse if it was a Use and a LateColdUse > if it was a ColdUse. If it's already some kind of late use then you don't > need to change it. Thanks for catching this. Fixed. Created attachment 307501 [details]
patch for landing.
Applies fixes from Fil's feedback. Let's get some EWS testing while I do perf testing.
JSC benchmarks say that perf is neutral. Now, we wait for EWS bots to finish testing. Created attachment 307505 [details]
x86_64 benchmark result.
Landed in r215531: <http://trac.webkit.org/r215531>. Comment on attachment 307501 [details] patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=307501&action=review > Source/JavaScriptCore/b3/B3CheckSpecial.cpp:117 > + if (Arg::isAnyDef(role)) { > + ASSERT(!optionalDefArgWidth); // There can only be one Def'ed arg. > + optionalDefArgWidth = width; I think what we want here is Arg::isLateDef. Fil, Mark, what do you think? This assertion fires on arm64 for: arm64: BranchMul32 U:G:32, U:G:32, U:G:32, S:G:32, S:G:32, ZD:G:32 /branch ResCond, Tmp, Tmp, Tmp, Tmp, Tmp (In reply to Saam Barati from comment #15) > Comment on attachment 307501 [details] > patch for landing. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307501&action=review > > > Source/JavaScriptCore/b3/B3CheckSpecial.cpp:117 > > + if (Arg::isAnyDef(role)) { > > + ASSERT(!optionalDefArgWidth); // There can only be one Def'ed arg. > > + optionalDefArgWidth = width; > > I think what we want here is Arg::isLateDef. > Fil, Mark, what do you think? > This assertion fires on arm64 for: > > arm64: BranchMul32 U:G:32, U:G:32, U:G:32, S:G:32, S:G:32, ZD:G:32 /branch > ResCond, Tmp, Tmp, Tmp, Tmp, Tmp Note that Scratch are defs: ``` static bool isAnyDef(Role role) { switch (role) { case Use: case ColdUse: case UseAddr: case LateUse: case LateColdUse: return false; case Def: case UseDef: case ZDef: case UseZDef: case EarlyDef: case EarlyZDef: case Scratch: return true; } ASSERT_NOT_REACHED(); } ``` |