[Webkit-unassigned] [Bug 65290] DFG speculative JIT does not implement load elimination for GetById

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=65290





--- Comment #7 from Filip Pizlo <fpizlo at apple.com>  2011-07-29 16:32:37 PST ---
(In reply to comment #6)
> (From update of attachment 102228 [details])
> 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.

Gosh, good catch on both counts!  My bad, I'll fix that.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.