[webkit-reviews] review denied: [Bug 94779] To reduce cost of converting type during modulo for integers : [Attachment 160089] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied hojong.han at samsung.com's request for
review:
Bug 94779: To reduce cost of converting type during modulo for integers
https://bugs.webkit.org/show_bug.cgi?id=94779

Attachment 160089: Patch
https://bugs.webkit.org/attachment.cgi?id=160089&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=160089&action=review


This patch will cause a regression on
fast/js/integer-division-neg2tothe32-by-neg1.html.  Did you run that test?

Please make sure your code covers all of the corner cases where integer
division fails, rather than just the obvious one (zero denominator).

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2539
> +    JITCompiler::Jump checkZero = m_jit.branch32(JITCompiler::Equal, op2GPR,
TrustedImm32(0));

Why are you doing it this way?	Why not m_jit.branchTest32(JITCompiler::Zero,
op2GPR)?

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2545
>> +	speculationCheck(Overflow, JSValueRegs(), NoNode, checkZero);
> 
> Did you verify what happens in the recovery path when this speculation fails?


This is completely wrong.  Why are you emitting a speculationCheck *after* the
call?  The speculationCheck() should be exactly where the branch is.  Please
hoist this up to the checkZero, above.