[webkit-reviews] review denied: [Bug 182216] [ESNext][BigInt] Implement "~" unary operation : [Attachment 357227] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 182216: [ESNext][BigInt] Implement "~" unary operation
https://bugs.webkit.org/show_bug.cgi?id=182216

Attachment 357227: Patch

https://bugs.webkit.org/attachment.cgi?id=357227&action=review




--- Comment #10 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 357227
  --> https://bugs.webkit.org/attachment.cgi?id=357227
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357227&action=review

Mostly LGTM, but I’m worried about how you lower to ArithBitNot vs ValueBitNot

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:383
> +	       setConstant(node, JSValue(~a));

I think you need a didFoldClobber here

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4929
> +	       if (isInt32Speculation(getPrediction()))

This reasoning is wrong. You’re saying you’ll produce an ArithBitNot if you
produce an int32, but you’ll always produce an Int32 I believe if you’re not a
big int. This logic should consider the operands, not the result. I think this
kind of logic is best done in Fixup. If you kept this code this way, I believe
you’re introducing an OSR exit loop every time you have some object type as an
input

> Source/JavaScriptCore/dfg/DFGOperations.cpp:368
> +	   RETURN_IF_EXCEPTION(scope, encodedJSValue());

No need for this extra branch. I think you can just RELEASE_AND_RETURN the
above operation