[webkit-reviews] review denied: [Bug 65290] DFG speculative JIT does not implement load elimination for GetById : [Attachment 102228] the patch (attempt to fix windows, again)

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 65290: DFG speculative JIT does not implement load elimination for GetById
https://bugs.webkit.org/show_bug.cgi?id=65290

Attachment 102228: the patch (attempt to fix windows, again)
https://bugs.webkit.org/attachment.cgi?id=102228&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102228&action=review


I think we need to blow aliases to same id on different bases, too, unless we
can prove they are not the same value.

> Source/JavaScriptCore/dfg/DFGAliasTracker.h:123
> +	       if (possibleAlias.child1() == node.child1()

This seems to only shoot down candidates for alias if they have the same base
in the graph, but not if they have different bases.  But different bases may
refer to the same object.
e.g. this will correctly prevent aliasing here (the two reads of a.y may read
different values due to the assignment of b):
function f1(a,b) { var x = a.y; a.y = b; return x + a.y; }
however I think this will fail on the following:
function f2(a,b,c) { var x = a.y; b.y = c; return x + a.y; }
This time we won't detect that the write to b.y aliases a.y, so the assignment
won't clear the alias & if the function is called with the same the object as a
& b, then the latter read of a.y will incorrectly alias the original value
rather than c.

I think we need to shoot down aliases in all cases where the identifier
matches?

> Source/JavaScriptCore/dfg/DFGJITCompiler.h:57
> +enum CompilerKind { Speculative, NonSpeculative, EitherCompiler };

JITCodeGenerator already has a member called m_isSpeculative, I don't think we
need both this & the CompilerKind enum.
It is probably better to use your enum since it is clearer & more descriptive,
so I'd replace the member bool with a CompilerKind.
But I think we do need this to be a member in some cases, so I think you can
remove passing CompilerKind as an augment to function on JITCompiler in a
number of cases, and just use the member instead.

The key thing here is that we should unify these; we shouldn't have two ways of
doing this.