Bug 196582 - [JSC] OSRExit recovery for SpeculativeAdd does not consier "A = A + A" pattern
Summary: [JSC] OSRExit recovery for SpeculativeAdd does not consier "A = A + A" pattern
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-03 17:15 PDT by Yusuke Suzuki
Modified: 2019-04-05 18:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.33 KB, patch)
2019-04-05 16:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (19.05 KB, patch)
2019-04-05 17:57 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-04-03 17:15:50 PDT
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.
Comment 1 Yusuke Suzuki 2019-04-04 21:20:43 PDT
We reliably reproduce this with --useDoublePredictionFuzzerAgent=1.
Comment 2 Yusuke Suzuki 2019-04-05 16:47:28 PDT
Created attachment 366859 [details]
Patch

WIP
Comment 3 Yusuke Suzuki 2019-04-05 17:57:29 PDT
Created attachment 366865 [details]
Patch
Comment 4 Saam Barati 2019-04-05 18:08:22 PDT
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()?
Comment 5 Saam Barati 2019-04-05 18:08:51 PDT
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"
Comment 6 Yusuke Suzuki 2019-04-05 18:13:54 PDT
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.
Comment 7 Saam Barati 2019-04-05 18:28:12 PDT
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.
Comment 8 Saam Barati 2019-04-05 18:28:39 PDT
(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.
Comment 9 Yusuke Suzuki 2019-04-05 18:43:07 PDT
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 :)
Comment 10 Yusuke Suzuki 2019-04-05 18:57:23 PDT
Committed r243959: <https://trac.webkit.org/changeset/243959>
Comment 11 Radar WebKit Bug Importer 2019-04-05 18:58:17 PDT
<rdar://problem/49664247>