<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Register usage optimization in mathIC when LHS and RHS are constants isn't configured correctly"
href="https://bugs.webkit.org/show_bug.cgi?id=160802#c33">Comment # 33</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Register usage optimization in mathIC when LHS and RHS are constants isn't configured correctly"
href="https://bugs.webkit.org/show_bug.cgi?id=160802">bug 160802</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=286147&action=diff" name="attach_286147" title="Patch">attachment 286147</a> <a href="attachment.cgi?id=286147&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=286147&action=review">https://bugs.webkit.org/attachment.cgi?id=286147&action=review</a>
<span class="quote">>>> 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?</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>