Created attachment 351418 [details] WIP - RFC Patch

Comment on attachment 351418 [details] WIP - RFC Patch Patch not ready yet.

@dinfuehr, @caitp, @xan and @guijemont could you take a look in this Patch before sending to review?

Created attachment 351721 [details] WIP - RFC Patch

Created attachment 351908 [details] Patch

Created attachment 351921 [details] Benchmarks These changes are perf neutral.

Ping Review

Ping Review

Ping Review

Comment on attachment 351908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351908&action=review I think our ArithProfile for `+` needs additional changes. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:594 > + ASSERT(node->binaryUseKind() == UntypedUse || node->binaryUseKind() == BigIntUse); Use DFG_ASSERT instead. > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:203 > changed |= mergePrediction(SpecInt32Only); > if (node->mayHaveDoubleResult()) > changed |= mergePrediction(SpecBytecodeDouble); Let's consider the polymorphic `+` site. It takes Number and BigInt. It can be like, `function add(a, b) { return a + b; }` and it is used in various places as a callback. Some functions pass Number, and some pass BigInt. In this case, ArithProfile says "Result includes non number". And this prediction says "String | Number". But it is not correct in this case. It should say "Number | BigInt". So, at least for `+`, I think ArithProfile needs to record additional result type information.

Comment on attachment 351908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351908&action=review Thx for the review! >> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:203 >> changed |= mergePrediction(SpecBytecodeDouble); > > Let's consider the polymorphic `+` site. It takes Number and BigInt. It can be like, `function add(a, b) { return a + b; }` and it is used in various places as a callback. Some functions pass Number, and some pass BigInt. > In this case, ArithProfile says "Result includes non number". And this prediction says "String | Number". But it is not correct in this case. It should say "Number | BigInt". > So, at least for `+`, I think ArithProfile needs to record additional result type information. Oops. Sorry I missed that. Nice Catch.

Created attachment 352640 [details] Patch

Created attachment 352648 [details] Patch

Comment on attachment 352648 [details] Patch Attachment 352648 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/9645190 New failing tests: stress/big-int-negate-jit.js.big-int-enabled apiTests

Comment on attachment 352648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352648&action=review Looks nice. But since I have found several issues, I would like to have another round of review. > Source/JavaScriptCore/bytecode/ArithProfile.cpp:41 > CCallHelpers::Jump isInt32 = jit.branchIfInt32(regs, mode); `done.append(jit.branchIfInt32(regs, mode))` seems simple. > Source/JavaScriptCore/bytecode/ArithProfile.cpp:47 > + CCallHelpers::Jump notBigInt = jit.branchIfNotBigInt(regs.payloadGPR()); It assumes the regs are JSCell, but it seems flaky. Putting branchIfNotCell before this is better. At that time, please pass `mode` to branchIfNotCell since we may not have tag register. > Source/JavaScriptCore/bytecode/ArithProfile.h:274 > void emitSetNonNumber(CCallHelpers&) const; I think renaming this term "NonNumber" in ArithProfile, DFG::Node flags etc. to "NonNumeric" is better since Numeric can include BigInt. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:926 > + node->mergeFlags(NodeMayHaveBigIntResult); I think ValueNegate and ArithNegate needs this flag too. > Source/JavaScriptCore/dfg/DFGNodeFlags.h:75 > +#define NodeMayHaveBigIntResult 0x100000 I think this flag should not be sparse: this should be cooperating with the other flags like NodeMayHaveNonNumberResult, NodeMayHaveDoubleResult etc. The value of this flag should be close to NodeMayHaveNonNumberResult. And NodeMayHaveNonIntResult should be updated since BigInt is NonInt result. > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:274 > // FIXME: We should add support to BigInt into speculatio > // https://bugs.webkit.org/show_bug.cgi?id=182470 This can be removed.

Created attachment 352873 [details] Patch

Comment on attachment 352648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352648&action=review Thx for the review! >> Source/JavaScriptCore/bytecode/ArithProfile.cpp:47 >> + CCallHelpers::Jump notBigInt = jit.branchIfNotBigInt(regs.payloadGPR()); > > It assumes the regs are JSCell, but it seems flaky. Putting branchIfNotCell before this is better. > At that time, please pass `mode` to branchIfNotCell since we may not have tag register. Done. BTW, I'm trying to create a test for such case, but I'm stuck to find a case where this code is emitted. Do you know when we emit such code? I tried OSR code generated due BadType and Overflow and neither one triggers the generation of this code. >> Source/JavaScriptCore/bytecode/ArithProfile.h:274 >> void emitSetNonNumber(CCallHelpers&) const; > > I think renaming this term "NonNumber" in ArithProfile, DFG::Node flags etc. to "NonNumeric" is better since Numeric can include BigInt. I agree. Done. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:926 >> + node->mergeFlags(NodeMayHaveBigIntResult); > > I think ValueNegate and ArithNegate needs this flag too. Done. >> Source/JavaScriptCore/dfg/DFGNodeFlags.h:75 >> +#define NodeMayHaveBigIntResult 0x100000 > > I think this flag should not be sparse: this should be cooperating with the other flags like NodeMayHaveNonNumberResult, NodeMayHaveDoubleResult etc. > The value of this flag should be close to NodeMayHaveNonNumberResult. And NodeMayHaveNonIntResult should be updated since BigInt is NonInt result. Makes sense. I updated it.

Created attachment 352874 [details] Benchmarks The performance is perf neutral according to results.

Created attachment 353619 [details] Patch

Comment on attachment 353619 [details] Patch r=me.

@Yusuke and @Keith thank you very much for the review!

Comment on attachment 353619 [details] Patch Clearing flags on attachment: 353619 Committed r237972: <https://trac.webkit.org/changeset/237972>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45896441>