[webkit-reviews] review granted: [Bug 196639] Clean up Int52 code and some bugs in it : [Attachment 366907] patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 196639: Clean up Int52 code and some bugs in it
https://bugs.webkit.org/show_bug.cgi?id=196639

Attachment 366907: patch

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




--- Comment #11 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 366907
  --> https://bugs.webkit.org/attachment.cgi?id=366907
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:16
> +	   However, this got confusing because we reused SpecInt32Only both for
JSValue
> +	   representations and Int52 representations. This actually lead to
some bugs.

I like this idea, it simplifies shouldSpeculateInt52 flow too, and now we do
not need to bother whether we should include SpecInt32Only in
shouldSpeculateInt52. (This should be handled by shouldSpeculateInt32 thing.
shouldSpeculateInt52 is now used to propagate Int52 format flow).

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:768
>		       break;

`shouldCheckOverflow(node->arithMode())` is always true here (this is also
asserted in DFGSpeculativeJIT.cpp).
We can remove this `|| !shouldCheckOverflow(node->arithMode()` part. By
dropping this part, the code looks well aligned to ArithAdd's one.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:737
> +	       setPrediction(speculationFromValue(m_currentNode->asJSValue()));

Nice.