[webkit-reviews] review denied: [Bug 186177] [BigInt] Add support to BigInt into ValueAdd : [Attachment 352648] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 18 06:38:39 PDT 2018
Yusuke Suzuki <yusukesuzuki at slowstart.org> has denied Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 186177: [BigInt] Add support to BigInt into ValueAdd
https://bugs.webkit.org/show_bug.cgi?id=186177
Attachment 352648: Patch
https://bugs.webkit.org/attachment.cgi?id=352648&action=review
--- Comment #15 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 352648
--> https://bugs.webkit.org/attachment.cgi?id=352648
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.
More information about the webkit-reviews
mailing list