Bug 181310 – Don't unconditionally lower op_inc to ArithAdd in the BytecodeParser

• Status: NEW
• Component: JavaScriptCore

[Saam Barati]

Otherwise we'll have an OSR exit loop when the input isn't of some specific set of types. For example, if the input is a string, it'll OSR exit in a loop.

[Saam Barati]

Created attachment 330499 [details]
patch

[Filip Pizlo]

Comment on attachment 330499 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330499&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4574
> -            set(srcDstVirtualRegister, makeSafe(addToGraph(ArithAdd, op, addToGraph(JSConstant, OpInfo(m_constantOne)))));
> +            if (op->hasNumberResult())
> +                set(srcDstVirtualRegister, makeSafe(addToGraph(ArithAdd, op, addToGraph(JSConstant, OpInfo(m_constantOne)))));
> +            else
> +                set(srcDstVirtualRegister, makeSafe(addToGraph(ValueAdd, op, addToGraph(JSConstant, OpInfo(m_constantOne)))));

Why not unconditionally lower to ValueAdd and let prediction/fixup figure it out?

[JF Bastien]

Comment on attachment 330499 [details]
patch

r=me

[Saam Barati]

Comment on attachment 330499 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330499&action=review

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4574
>> +                set(srcDstVirtualRegister, makeSafe(addToGraph(ValueAdd, op, addToGraph(JSConstant, OpInfo(m_constantOne)))));
> 
> Why not unconditionally lower to ValueAdd and let prediction/fixup figure it out?

That's what I originally thought as well. I decided to just follow what op_add does though. I think we should either change both or keep this as is.

[Saam Barati]

Comment on attachment 330499 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330499&action=review

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4574
>>> +            if (op->hasNumberResult())
>>> +                set(srcDstVirtualRegister, makeSafe(addToGraph(ArithAdd, op, addToGraph(JSConstant, OpInfo(m_constantOne)))));
>>> +            else
>>> +                set(srcDstVirtualRegister, makeSafe(addToGraph(ValueAdd, op, addToGraph(JSConstant, OpInfo(m_constantOne)))));
>> 
>> Why not unconditionally lower to ValueAdd and let prediction/fixup figure it out?
> 
> That's what I originally thought as well. I decided to just follow what op_add does though. I think we should either change both or keep this as is.

Oh wow, I think my code is horribly wrong. This will make:

let x = "foo";
x++;

have x === "foo1" at the end!

Obviously I need something else here.

[Saam Barati]

This isn't super high priority. I'll get back to it at some point.