Closed Bug 1276181 Opened 8 years ago Closed 2 years ago

Fold loads depending on multiple stores

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: h4writer, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 1255008, 1275248
Attached patch WIP (obsolete) — Splinter Review
This is a working WIP. This folds multiple store dependencies on loads.
richards (5), raytrace (2), typescript (few, recompilations?)
Assignee: nobody → hv1989
Attached patch WIPSplinter Review
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 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)
(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.
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.
Priority: -- → P5

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: hv1989 → nobody

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.

Attachment

General

Created:
Updated:
Size: