[webkit-reviews] review denied: [Bug 198253] [JSC] ResultType implementation is wrong for bit ops, and ends up making ArithDiv in DFG Int32 fast path even if Baseline constantly produces Double result : [Attachment 378452] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 198253: [JSC] ResultType implementation is wrong for bit ops, and ends up
making ArithDiv in DFG Int32 fast path even if Baseline constantly produces
Double result
https://bugs.webkit.org/show_bug.cgi?id=198253

Attachment 378452: Patch

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




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 378452
  --> https://bugs.webkit.org/attachment.cgi?id=378452
Patch

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

I think this is the wrong place to do this fix.  This method says
bigIntOrInt32Type().  It's misleading to have it also return TypeMaybeNumber. 
The real issue is in forBitOp(), which happens to be the only client of this
method.  I think we should just do the fix in forBitOp() and remove
bigIntOrInt32Type().  Alternatively, rename bigIntOrInt32Type() to
bigIntOrInt32TypeOrNumber() if you think there will be other clients of it
coing soon.

> Source/JavaScriptCore/ChangeLog:3
> +	   [JSC] ResultType implementation is wrong for bit ops, and ends up
making ArithDiv in DFG Int32 fast path even if Baseline constantly produces
Double result

I suggest rephrasing as "making ArithDiv take the DFG Int32 fast path".

> Source/JavaScriptCore/ChangeLog:8
> +	   ResultType of bitwise operation needs including TypeMaybeNumber.
TypeInt32 is something like a flag indicating the number looks like a int32.

I suggest rephrasing as "needs to include TypeMaybeNumber".