[Webkit-unassigned] [Bug 160802] Register usage optimization in mathIC when LHS and RHS are constants isn't configured correctly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=160802

--- Comment #32 from Caio Lima <ticaiolima at gmail.com> ---
Comment on attachment 286147
  --> https://bugs.webkit.org/attachment.cgi?id=286147
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
>> +    if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand)) {
> 
> Sorry, I was totally wrong about this code. We don't need to check Generator::isLeftOperandValidConstant(leftOperand) here since
> the code above makes sure that not both operands can be constant. Sorry for incorrectly suggesting that before.
> Instead, what I'd do is just have an assertion above like so (by the other related assertion):
> ASSERT(!(Generator::isLeftOperandValidConstant(leftOperand) && Generator::isRightOperandValidConstant(rightOperand)));

Actually, IMHO I think it is correct. The code previously is just considering  "isInt32Constant" and coincidently our current JITMathICs are just considering Int32Constant as possible constant operand. However, one of them can potentially consider a rule with "isNumberConstant" or even RHS or LHS as always valid constant  and "(left|right)Child->isInt32Constant" is not considering it. I think this design better because we leave the valid constant rule in JITBlahGenerator responsibility. Does it make sense to you?

>> Source/JavaScriptCore/jit/JITArithmetic.cpp:741
>> +    if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand))
> 
> Ditto here. I'd do the same thing.

Ditto above.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160825/a35dec14/attachment.html>