[webkit-reviews] review denied: [Bug 220322] BooleanConstructor should be inlined in DFG / FTL : [Attachment 416996] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 220322: BooleanConstructor should be inlined in DFG / FTL
https://bugs.webkit.org/show_bug.cgi?id=220322

Attachment 416996: Patch

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




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

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

Looks good. But putting r- since I found several bugs.

> Source/JavaScriptCore/ChangeLog:20
> +	   * dfg/DFGAbstractInterpreterInlines.h:

Yeah, please add notes about masquerade as undefined thing in ChangeLog, and
please add tests for that :)
That's awesome.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1538
>	   GPRTemporary result(this, Reuse, value);
> -	   m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
> +	   if (invert)
> +	       m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
>	   booleanResult(result.gpr(), node);

Reuse is "Reuse if it is possible" If node->child1's use count is not zero at
this point, GPRTemporary allocates a new register.
So, if you do not move value to result, result can contain a garbage.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1905
>	       GPRTemporary result(this, Reuse, value);

While this is old code, I wonder if SpeculateBooleanOperand is correct here.
Can you check what format is generated from SpeculateBooleanOperand? JSValue?
Or 0/1?
Maybe, in 64bit, we never use DataFormatBoolean in DFG?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1924
> +	   if (invert)
> +	       m_jit.xor64(TrustedImm32(JSValue::ValueTrue), result.gpr());

Is it correct? If `invert` is false, is result the JSValue format? I think
result is 0/1 at this point, and it is not JSBoolean.