Closed
Bug 1276181
Opened 8 years ago
Closed 2 years ago
Fold loads depending on multiple stores
Categories
(Core :: JavaScript Engine: JIT, defect, P5)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: h4writer, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
20.22 KB,
patch
|
Details | Diff | Splinter Review |
FlowAA keeps track of all stores a load depends on. In the current code this only leads to better dependencies when there is only one dependency since we throw the information away if there are multiple dependencies. This bug is about leveraging that and make the folding aware of multiple dependencies and act on it.
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Comment 1•8 years ago
|
||
This is a working WIP. This folds multiple store dependencies on loads. richards (5), raytrace (2), typescript (few, recompilations?)
Assignee: nobody → hv1989
Reporter | ||
Comment 2•8 years ago
|
||
Still a WIP, but getting closer to the real deal! Soonish we will be able to fold loads that depend on multiple stores!! I got an infrastructural question that could use some discussion. 1) Do we perhaps need to do the "foldsToStore" part in AA pass itself, instead of in GVN? Yes: - MIR.cpp is already large and foldsToStore is becoming larger and larger. Now I added functionality to place phis at appropriate places. - GVN handles mostly direct dependencies (operands). Folding stores/loads are actually implicit dependencies. Is it a misfit and shouldn't we do this, No: - AA doesn't have the newest information. As a result we will optimize less, except if we would iterate AA and GVN a couple of times (till fixed point). Which also doesn't seem wanted Personally I would keep it in GVN. 2) Do you have any questions or concerns about this approach?
Attachment #8757241 -
Attachment is obsolete: true
Attachment #8758202 -
Flags: feedback?(jdemooij)
Comment 3•8 years ago
|
||
Comment on attachment 8758202 [details] [diff] [review] WIP Sorry for the delay. What if we make it a new pass? I agree it doesn't really belong as part of GVN (especially if we adds phis etc), but it also seems wrong to have it as part of AA, that pass always just annotated the MIR without changing it directly and it'd be nice to keep that.
Attachment #8758202 -
Flags: feedback?(jdemooij)
Reporter | ||
Comment 4•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > Comment on attachment 8758202 [details] [diff] [review] > WIP > > Sorry for the delay. > > What if we make it a new pass? I agree it doesn't really belong as part of > GVN (especially if we adds phis etc), but it also seems wrong to have it as > part of AA, that pass always just annotated the MIR without changing it > directly and it'd be nice to keep that. Okay, makes sense.
Comment 5•8 years ago
|
||
Just noting my desire for this optimization not to propagate value information about TypedArray locations across atomic operations. I expect the machinery we currently use will prevent problems (see eg MLoadUnboxedScalar::getAliasSet) but if it doesn't we'll need to talk.
Updated•8 years ago
|
Priority: -- → P5
Comment 6•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: hv1989 → nobody
Comment 7•2 years ago
|
||
This is an improvement to FlowAliasAnalysis, which was removed years ago (bug 1455280).
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•