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
Comment on attachment 227411 [details] Patch r=me
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.
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.
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.
> 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?
(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.
I have a local patch to remove the bytecode parser's constant folder and I'm testing it now.
(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.
Created attachment 227476 [details] the patch
Comment on attachment 227476 [details] the patch r=me
Landed in http://trac.webkit.org/changeset/166095
(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
(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.
(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