...
Created attachment 344077 [details] WIP -It starts This Patch is not complete yet.
Created attachment 351876 [details] WIP - Patch
Comment on attachment 351876 [details] WIP - Patch Attachment 351876 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9503455 New failing tests: webgl/2.0.0/conformance/uniforms/out-of-bounds-uniform-array-access.html webgl/1.0.2/conformance/uniforms/out-of-bounds-uniform-array-access.html
Created attachment 351880 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 351876 [details] WIP - Patch Attachment 351876 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9504348 New failing tests: webgl/2.0.0/conformance/uniforms/out-of-bounds-uniform-array-access.html webgl/1.0.2/conformance/uniforms/out-of-bounds-uniform-array-access.html
Created attachment 351890 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 351914 [details] WIP - Patch
Comment on attachment 351914 [details] WIP - Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119 > + node->setOp(ArithSub); You shoud use setOpAndDefaultFlags since it does not clear MustGenerate. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:126 > + node->setOp(ArithSub); Ditto.
Comment on attachment 351914 [details] WIP - Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review Thx @Yusuke for the informal review. I left some questions about your review. >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119 >> + node->setOp(ArithSub); > > You shoud use setOpAndDefaultFlags since it does not clear MustGenerate. IIRC, I think using this will make this code wrong, because setOpAndDefaultFlags will reset this node result and ```attemptToMakeIntegerAdd``` sets properly expected result in some cases. >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:126 >> + node->setOp(ArithSub); > > Ditto. I'm not sure if you mean to remove NodeResultDouble and use ```setOpAndDefaultFlags```, but ArithSub default flag is ```NodeResultNumber``` instead of ```NodeResultDouble```. Could you confirm that? Using ```setOpAndDefaultFlags``` instead of ```setOp + setResult``` makes a bunch of test crash.
Comment on attachment 351914 [details] WIP - Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review >>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119 >>> + node->setOp(ArithSub); >> >> You shoud use setOpAndDefaultFlags since it does not clear MustGenerate. > > IIRC, I think using this will make this code wrong, because setOpAndDefaultFlags will reset this node result and ```attemptToMakeIntegerAdd``` sets properly expected result in some cases. Make sense. Then, you need to clear MustGenerate by using clearFlags(NodeMustGenerate). >>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:126 >>> + node->setOp(ArithSub); >> >> Ditto. > > I'm not sure if you mean to remove NodeResultDouble and use ```setOpAndDefaultFlags```, but ArithSub default flag is ```NodeResultNumber``` instead of ```NodeResultDouble```. Could you confirm that? Using ```setOpAndDefaultFlags``` instead of ```setOp + setResult``` makes a bunch of test crash. Ditto. You need to clear NodeMustGenerate flag.
Comment on attachment 351914 [details] WIP - Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review >>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119 >>>> + node->setOp(ArithSub); >>> >>> You shoud use setOpAndDefaultFlags since it does not clear MustGenerate. >> >> IIRC, I think using this will make this code wrong, because setOpAndDefaultFlags will reset this node result and ```attemptToMakeIntegerAdd``` sets properly expected result in some cases. > > Make sense. Then, you need to clear MustGenerate by using clearFlags(NodeMustGenerate). After looking into the code of attemptToMakeIntegerAdd, I've just understood the problem. ArithSub can have checks (for overflow), so, MustGenerate cannot be cleared easily. (Only Unchecked situation can drop MustGenerate). Because the original ArithSub node has MustGenerate flag anyway, clearing it would be done in a separate patch.
Created attachment 352395 [details] Patch
Comment on attachment 352395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352395&action=review r=me with nit. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:595 > + ASSERT(node->binaryUseKind() == UntypedUse || node->binaryUseKind() == BigIntUse); Let's use DFG_ASSERT. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:125 > + ASSERT(Node::shouldSpeculateNumberOrBooleanExpectingDefined(child1.node(), child2.node())); Let's use DFG_ASSERT.
Created attachment 352666 [details] Patch
Thx for the review!
Comment on attachment 352666 [details] Patch Clearing flags on attachment: 352666 Committed r237242: <https://trac.webkit.org/changeset/237242>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45358866>
Re-opened since this is blocked by bug 190701
Created attachment 352768 [details] Patch
@Yusuke. I updated this patch comparing to the previous one removing the assert "Node::shouldSpeculateNumberOrBooleanExpectingDefined(child1.node(), child2.node())" from DFGFixupPhase.cpp. The point is that the following assert is not true. I created the test ```JSTests/stress/value-sub-spec-none-case.js``` to show an edge case that will cause the assertion to fail. In this test, ValueSub will have one operand as SpecNone and other as Double. Even though we are fixing edge as double or boolean, this fallback is still safe, because such cases will generate ForceOSRExit before the execution of ValueSub/ArithSub.
Comment on attachment 352768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352768&action=review r=me > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:120 > + // FIXME: [DFG] Clear ArithSub when ArithMode is Unchecked > + // https://bugs.webkit.org/show_bug.cgi?id=190607 Ditto. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:327 > + // FIXME: [DFG] Clear ArithSub when ArithMode is Unchecked Fix the description to "Clear ArithSub's NodeMustGenerate when ArithMode is unchecked". And `[DFG]` is not necessary.
Created attachment 352782 [details] Patch
Thx agin for the review.
Comment on attachment 352782 [details] Patch Clearing flags on attachment: 352782 Committed r237285: <https://trac.webkit.org/changeset/237285>
Comment on attachment 352782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352782&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:596 > clobberWorld(); this isn't true when we're BigIntUse > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:600 > + if (node->binaryUseKind() == BigIntUse) > + setTypeForNode(node, SpecBigInt); > + else > + setTypeForNode(node, SpecString | SpecBytecodeNumber | SpecBigInt); We should do some constant folding here. Profiling information may not be perfect, and AI could prove these values as constants. > Source/JavaScriptCore/dfg/DFGClobberize.h:642 > + case ValueSub: This should be PureValue when BigIntUse. > Source/JavaScriptCore/dfg/DFGDoesGC.cpp:102 > + case ValueSub: With the other changes I propose, this should return `true` for BigIntUse > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:108 > + fixEdge<BigIntUse>(child2); We should clear MustGenerate here I believe. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:127 > + fixDoubleOrBooleanEdge(node->child1()); > + fixDoubleOrBooleanEdge(node->child2()); > + node->setOp(ArithSub); Should we clear MustGenerate here?
Comment on attachment 352782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352782&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:596 >> clobberWorld(); > > this isn't true when we're BigIntUse We changed this on ToT already. >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:600 >> + setTypeForNode(node, SpecString | SpecBytecodeNumber | SpecBigInt); > > We should do some constant folding here. Profiling information may not be perfect, and AI could prove these values as constants. This is good to know. I'll do that in coming patches. >> Source/JavaScriptCore/dfg/DFGClobberize.h:642 >> + case ValueSub: > > This should be PureValue when BigIntUse. It is already done on ToT. >> Source/JavaScriptCore/dfg/DFGDoesGC.cpp:102 >> + case ValueSub: > > With the other changes I propose, this should return `true` for BigIntUse I'll change that. Right now we actually have wrong rules for every Op. >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:108 >> + fixEdge<BigIntUse>(child2); > > We should clear MustGenerate here I believe. I think it is ok. >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:127 >> + node->setOp(ArithSub); > > Should we clear MustGenerate here? I'm not sure. Can we have issues with neg zero here?