[webkit-reviews] review denied: [Bug 186177] [BigInt] Add support to BigInt into ValueAdd : [Attachment 352648] Patch

[bugzilla-daemon at webkit.org]

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.