Bug 130576 – Constants folded by DFG::ByteCodeParser should not be dead.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

The testcase below should print "specFailArg", but it prints "0" instead.

function test_true_open()
{
    function passThrough(arg) {
        var a = true;
        var b = arg * 0.1;
        if (a)
            return arg;
        else
            return 0;
    }
    for (var i = 0; i < 1000; i++) {
        passThrough(i);
    }
    var specFailArg = "specFailArg";
    print(passThrough(specFailArg));
}


Generated bytecode for function passThrough():
[   0] enter
[   1] mov               loc0, True(@k0)
[   4] mul               loc1, arg1, Double: 4591870180066957722, 0.100000(@k1)
[   9] jfalse            loc0, 7(->16)
[  12] ret               arg1
[  14] jmp               4(->18)
[  16] ret               Int32: 0(@k2)
[  18] ret               Undefined(@k3)


OSR exit occurs when passThrough() is called with the argument "specFailArg", at dfg code generated by bc#4.
However the OSR exit thunk writes Undefined to loc0, 
so when executing baseline jit code generated by bc#9, it reads Undefined value and returns 0.


Generated DFG graph for function passThrough():
Block #0 (bc#0):  (OSR target)
  Predecessors:
  Dominated by: #0
  Dominates: #0
  vars before: arg1:(Top, TOP, TOP, TOP) arg0:(Top, TOP, TOP, TOP)
  var links: arg1:@1
   0:           <!0:->  Phantom(MustGen|CanExit, bc#0)
   1:           < 2:->  SetArgument(IsFlushed, arg1(B~<Int32>/FlushedJSValue), machine:arg1, W:SideState, bc#0)  predicting Int32
   2:           <!0:->  Phantom(MustGen|CanExit, Other, bc#0)
   3:  skipped  < 0:->  ZombieHint(loc0, W:SideState, bc#0)
   4:           <!0:->  Phantom(MustGen|CanExit, bc#0)
   5:  skipped  < 0:->  ZombieHint(loc1, W:SideState, bc#0)
   6:           <!0:->  Phantom(MustGen|CanExit, bc#0)
   7:           <!0:->  Phantom(MustGen|CanExit, Bool, bc#1)
   8:  skipped  < 0:->  ZombieHint(loc0, W:SideState, bc#1)
   9:           <!0:->  Phantom(MustGen|CanExit, bc#4)
  10:           < 3:-1> GetLocal(@1, JS|UseAsOther, Int32, arg1(B~<Int32>/FlushedJSValue), machine:arg1, R:Variables(7), bc#4)  predicting Int32
  11:           <!0:->  Phantom(MustGen|CanExit, Nonintasdouble, bc#4)
  26:           <!0:->  Phantom(Check:Number:@10<Int32>, MustGen|CanExit, Int52asdouble, bc#4)
  12:           <!0:->  Phantom(MustGen|CanExit, Int52asdoubleNonintasdouble, bc#4)
  27:           <!0:->  Phantom(@10<Int32>, MustGen, bc#4)
  13:  skipped  < 0:->  ZombieHint(loc1, W:SideState, bc#4)
  14:           <!0:->  Phantom(MustGen|CanExit, bc#9)
  16:           <!0:->  Flush(@1, MustGen|IsFlushed, arg1(B~<Int32>/FlushedJSValue), machine:arg1, W:SideState, bc#12)  predicting Int32
  17:           <!0:->  Phantom(MustGen|CanExit, Int32, bc#12)
  18:           <!0:->  Return(@10<Int32>, MustGen, W:SideState, bc#12)
  vars after:
  var links: arg1:@10<Int32>


The nodes generated by bc#1 becomes dead,
as a result of constant folding in DFG::ByteCodeParser().

I think DFG::ByteCodeParser should tell the backend optimization phase that the folded constant should not be dead.

Created attachment 227411 [details]
Patch

new patch

[Geoffrey Garen]

Comment on attachment 227411 [details]
Patch

r=me

[Filip Pizlo]

I don't think this is the right approach. We already have machinery for inserting phantoms in places like this and for avoiding the kind of constant folding that would lead to errors. Let's not add complexity here.

[Filip Pizlo]

The correct solution is almost certainly to just disable constant folding in the DFG. This should be perf neutral since we have all of the right folding later in the pipe.

[Filip Pizlo]

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. 

- your test is incomplete. It doesn't cover all of the cases where you added phantoms. At a minimum you should give this 100% test coverage. That's a hard prerequisite for landing this patch.

[Geoffrey Garen]

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

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

[Filip Pizlo]

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

[Filip Pizlo]

I have a local patch to remove the bytecode parser's constant folder and I'm testing it now.

[Filip Pizlo]

(In reply to comment #8)
> I have a local patch to remove the bytecode parser's constant folder and I'm testing it now.

Yup, as I expected, there is no perf penalty to getting rid of the bytecode parser's folder.  The real folder can do everything that the parser can do and our optimization fixpoint takes care of the rest.  The fact that the JIT is concurrent further removes any benefit from whatever small compile-time win this could have ever been.

[Filip Pizlo]

Created attachment 227476 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 227476 [details]
the patch

r=me

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/166095

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> Landed in http://trac.webkit.org/changeset/166095

Is "constand" a typo?  Should be "constant"?

> Source/JavaScriptCore/tests/stress/constand-folding-osr-exit.js

[Filip Pizlo]

(In reply to comment #13)
> (In reply to comment #12)
> > Landed in http://trac.webkit.org/changeset/166095
> 
> Is "constand" a typo?  Should be "constant"?
> 
> > Source/JavaScriptCore/tests/stress/constand-folding-osr-exit.js

Oh, wow, yeah that's a typo.  I will fix.

[Filip Pizlo]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Landed in http://trac.webkit.org/changeset/166095
> > 
> > Is "constand" a typo?  Should be "constant"?
> > 
> > > Source/JavaScriptCore/tests/stress/constand-folding-osr-exit.js
> 
> Oh, wow, yeah that's a typo.  I will fix.

Fixed in http://trac.webkit.org/changeset/166133