Bug 256872 - DFG::PutStackSinkingPhase inserts PutStack with wrong value that from ssaCalculator.reachingDefAtHead
Summary: DFG::PutStackSinkingPhase inserts PutStack with wrong value that from ssaCalc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Degazio
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-05-16 20:08 PDT by YuHao Hu
Modified: 2023-07-07 14:41 PDT (History)
1 user (show)

See Also:


Attachments
poc with wrong result (937 bytes, text/plain)
2023-05-16 20:08 PDT, YuHao Hu
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description YuHao Hu 2023-05-16 20:08:37 PDT
Created attachment 466374 [details]
poc with wrong result

run with:
./WebKitBuild/Debug/bin/jsc --useConcurrentJIT=0 --jitPolicyScale=0.001 test.js


`opt` function's IR after PutStack sinking:
```
#0
D@22 : JSConstant(3333)

#6
D@68 : Phi(...)
D@190: MovHint(D@68,arg1)
D@162: PutStack(D@22, arg1)      <-- inserted by PutStackSinkingPhase, which is wrong
D@126: ArithAdd(CheckOverflow)   <-- osr exit here
D@131: Return(D@68)
```

expected output:
1111

actual output:
3333

I think it's because `ssaCalculator.reachingDefAtHead` found the value `3333` from the dominator #0, but not the missing phi(probably around D@68), leading to a misjudgment of the variable reference. When analyzing the `then branch` of the first if statement, `a` is marked as `DeadFlush`, and the `functor` argument of `ssaCalculator.computePhis` (in DFGPutStackSinkingPhase.cpp) returns nullptr. The nullptr prevents subsequent ssa calculation, so the `phi` on the last block is not created.
Comment 1 Radar WebKit Bug Importer 2023-05-23 20:09:16 PDT
<rdar://problem/109752832>
Comment 2 David Degazio 2023-07-06 15:37:50 PDT
Pull request: https://github.com/WebKit/WebKit/pull/15613
Comment 3 EWS 2023-07-07 14:41:55 PDT
Committed 265866@main (8495ff2f3399): <https://commits.webkit.org/265866@main>

Reviewed commits have been landed. Closing PR #15613 and removing active labels.