<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#c13">Comment # 13</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:mark.lam@apple.com" title="Mark Lam <mark.lam@apple.com>"> <span class="fn">Mark Lam</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=160802#c12">comment #12</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=285914&action=diff" name="attach_285914" title="Patch">attachment 285914</a> <a href="attachment.cgi?id=285914&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=285914&action=review">https://bugs.webkit.org/attachment.cgi?id=285914&action=review</a>
>
> >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3474
> >> +
> >
> > Thanks for identifying the issue. A few points come to mind:
> >
> > 1. I think this is brittle and hacky because you're setting the SnippetOperands here and then overriding it again later in the generator initialization. However, I'm not entirely satisfied with any alternatives that I can think of at the moment either (need more consideration). At minimum, we should have a debug build flag in the Generator that says that it isn't (or its snippetOperands aren't) initialized yet, and assert on that flag in JITMathIC::isLeftOperandValidConstant() and isRightOperandValidConstant().
> >
> > 2. Can you add some tests that shows that this is broken (unless existing tests can already cover this)? This will come in handy if someone accidentally breaks this in the future. The assertions suggested in (1) should make it easier to test.
> >
> > Thanks.
>
> Caio, I just realized there is a weird dependency here between populating
> the left/right regs, and creating the Generator.
> I agree with mark that it seems brittle to set them here and then overwrite
> them. My suggestion, to a less brittle approach,
> is to simply have isLeftOperandValidConstant(...) and
> isRightOperandValidConstant(...) take SnippetOperand as an argument
> to the function. This won't allow callers to mess this up. You can just make
> them static functions on the JITBlahGenerator. Also, if you make that
> change, you'll need
> to make sure we don't consider left *and* right as constants. We only allow
> for one or the other being a constant. So that means you'll have to change
> the
> if statement below to be an else if</span >
I do like this idea of making these methods static and explicitly passing the SnippetOperand to them instead.
<span class="quote">> Mark, to address your point (2), this was just removing an optimization, it
> wasn't breaking existing tests AFAIK.</span >
Good point.</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>