[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 #33 from Saam Barati <sbarati at apple.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?

I see your argument. I agree with your sentiment, however, I don't completely agree with your assertion that it's JITBlahGenerator's responsibility at the moment to determine what a valid constant is (I would argue that it's partially its responsibility). Currently, the caller of such functions decides whether or not to put constants into the SnippetOperands. I think it's cleaner for the code to go full in on this assumption, and back it up with assertions. I'm not totally against your argument, but I'm not a huge fan of having the code pretend a condition can hold when it can't. I think it's better to just have the code assert that the condition holds, and if we ever decide to improve how constants flow into JITBlahGenerator, we can remove the assertion.

-- 
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/20160829/3bed0dc6/attachment-0001.html>