Bug 196582 – [JSC] OSRExit recovery for SpeculativeAdd does not consier "A = A + A" pattern

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

[Yusuke Suzuki]

We have a code like this.

   4297         SpeculateInt32Operand op1(this, node->child1());
   4298         SpeculateInt32Operand op2(this, node->child2());
   4299         GPRTemporary result(this, Reuse, op1, op2);
   4300
   4301         GPRReg gpr1 = op1.gpr();
   4302         GPRReg gpr2 = op2.gpr();
   4303         GPRReg gprResult = result.gpr();
   4304
   4305         if (!shouldCheckOverflow(node->arithMode()))
   4306             m_jit.add32(gpr1, gpr2, gprResult);
   4307         else {
   4308             MacroAssembler::Jump check = m_jit.branchAdd32(MacroAssembler::Overflow, gpr1, gpr2, gprResult);
   4309
   4310             if (gpr1 == gprResult)
   4311                 speculationCheck(Overflow, JSValueRegs(), 0, check, SpeculationRecovery(SpeculativeAdd, gprResult, gpr2));
   4312             else if (gpr2 == gprResult)
   4313                 speculationCheck(Overflow, JSValueRegs(), 0, check, SpeculationRecovery(SpeculativeAdd, gprResult, gpr1));
   4314             else
   4315                 speculationCheck(Overflow, JSValueRegs(), 0, check);

And recovery side code is the following.

   1114             jit.sub32(recovery->src(), recovery->dest());


If gpr1 = gpr2 = gprResult (this can happen if child1 and child2 are the same and GPRTemporary Reuse works), the result becomes zero accidentally.

[Yusuke Suzuki]

We reliably reproduce this with --useDoublePredictionFuzzerAgent=1.

[Yusuke Suzuki]

Created attachment 366859 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 366865 [details]
Patch

[Saam Barati]

Comment on attachment 366865 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        We do not need to have this mechanism in FTL (because FTL handles registers and stack places better, we do not need to
> +        have such a peephole optimization), so only DFG has this bug. This is found by injecting Double prediction into value
> +        profiling.

I wouldn't even say this. The FTL would also benefit from this as it alleviates register pressure.

Instead, I'd just say you're removing dead code.

> Source/JavaScriptCore/dfg/DFGOSRExit.h:62
> +    SpeculativeAddSelf,

Can't this just be detected by asking if src() == dst()?

[Saam Barati]

Comment on attachment 366865 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:1129
> +            // If A + A = A (int32_t) overflows, A can be recovereted by ((static_cast<int32_t>(A) >> 1) ^ 0x8000000).

typo: "recovereted" => "recovered"

[Yusuke Suzuki]

Comment on attachment 366865 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:18
>> +        profiling.
> 
> I wouldn't even say this. The FTL would also benefit from this as it alleviates register pressure.
> 
> Instead, I'd just say you're removing dead code.

OK, I'll rephrase this part.

>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:1129
>> +            // If A + A = A (int32_t) overflows, A can be recovereted by ((static_cast<int32_t>(A) >> 1) ^ 0x8000000).
> 
> typo: "recovereted" => "recovered"

Fixed.

>> Source/JavaScriptCore/dfg/DFGOSRExit.h:62
>> +    SpeculativeAddSelf,
> 
> Can't this just be detected by asking if src() == dst()?

We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate.

[Saam Barati]

Comment on attachment 366865 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGOSRExit.h:62
>>> +    SpeculativeAddSelf,
>> 
>> Can't this just be detected by asking if src() == dst()?
> 
> We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate.

Kind of. SpeculativeAdd just means A and B are registers. It doesn't care what registers are. Immediate makes more sense because it means we don't need a register for the purposes of recovery.

[Saam Barati]

(In reply to Saam Barati from comment #7)
> Comment on attachment 366865 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366865&action=review
> 
> >>> Source/JavaScriptCore/dfg/DFGOSRExit.h:62
> >>> +    SpeculativeAddSelf,
> >> 
> >> Can't this just be detected by asking if src() == dst()?
> > 
> > We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate.
> 
> Kind of. SpeculativeAdd just means A and B are registers. It doesn't care
> what registers are. Immediate makes more sense because it means we don't
> need a register for the purposes of recovery.

Anyways, I don't have a super strong opinion here. I'll let you decide what you prefer.

[Yusuke Suzuki]

Comment on attachment 366865 [details]
Patch

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

>>>>> Source/JavaScriptCore/dfg/DFGOSRExit.h:62
>>>>> +    SpeculativeAddSelf,
>>>> 
>>>> Can't this just be detected by asking if src() == dst()?
>>> 
>>> We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate.
>> 
>> Kind of. SpeculativeAdd just means A and B are registers. It doesn't care what registers are. Immediate makes more sense because it means we don't need a register for the purposes of recovery.
> 
> Anyways, I don't have a super strong opinion here. I'll let you decide what you prefer.

Personally, I like having a separate types here because these types are corresponding to the way how to recover the value from the given arguments.
By having separate types, I think our recovery side code can be simple, which has the `switch` on how to recover the value, and each clause maintains the recovery method corresponding to this type :)

[Yusuke Suzuki]

Committed r243959: <https://trac.webkit.org/changeset/243959>

[Radar WebKit Bug Importer]

<rdar://problem/49664247>