[Webkit-unassigned] [Bug 130576] Constants folded by DFG::ByteCodeParser should not be dead.

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Filip Pizlo <fpizlo at apple.com>  2014-03-21 11:52:47 PST ---
(In reply to comment #6)
> > Thought about it more. I think that your approach can work, but:
> > 
> > - we already have a ton of complexity in the byte code parser's folder and we have bugs because of it. Your patch adds complexity. 
> > 
> > - it's not at all clear that this folder is buying us any performance. Hence it may just be a complex piece of code that is only getting more complex that literally buys nothing but pain. 
> 
> Can you be more concrete in this feedback?

Before we get into the value of the bytecode parser's constant folder, we need to appreciate that this patch is wrong for at least two reasons:

- It doesn't actually fix the bug.  It adds Phantom arguments to the Phantoms we already had as placeholders, but fails to add Phantoms with those same arguments in case we turn the branches into jumps.  It kind of fixes less than half of the bug.  The real solution would have hoisted the addToGraph(Phantom, ...) out of the fallthrough-versus-jump branch.

- It doesn't actually test the fix.  The patch adds Phantoms in loads of places in the bytecode parser but only adds a test for one very specific sub-case.

> 
> Are you suggesting an alternative patch that removes all the constant folding from the bytecode parser, or something else?

There are many ways of solving this problem - either removing the constant folder or something else.  We should investigate which is most valuable.  The bytecode parser's constant folder was already a hackpile, and if we start going in the direction of a proper fix, we'll probably turn it into an even worse hackpile.  We need to establish that it's worth it for some reason (compile times?) before doing it.

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