[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 #12 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 285914
  --> https://bugs.webkit.org/attachment.cgi?id=285914
Patch

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

>> 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

Mark, to address your point (2), this was just removing an optimization, it wasn't breaking existing tests AFAIK.

-- 
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/20160812/7ef39b60/attachment-0001.html>