Closed Bug 983987 Opened 11 years ago Closed 11 years ago

PJS: Parallel analysis can be foiled by dead code

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently the parallel analysis runs before the dead code removal, which means that we sometimes fail to compile (or execute) due to instructions that -- while not safe for parallel execution -- are also dead. One place this crops up is MNewDerivedTypedObjects, which are often generated by IonBuilder without any uses. One way to fix this is just to do a quick dead code pass before running parallel analysis (that's what I did locally, but i've not tested it widely yet, maybe it fails for some reason, though I think it ought to work). Another way would just be to specialize the MNewDerivedTypedObject case in the parallel safety analysis. Either way this blocks bug 966567.
Blocks: PJS, 966567
Are these MNewDerivedTypedObjects dead in the entry basic block?
Assignee: nobody → nmatsakis
Attached patch Bug983987.diff (obsolete) — Splinter Review
Attachment #8391785 - Flags: review?(shu)
Shu: Sometimes, but the main point is that they lie along code paths that will be taken. So when the execution gets there, we bail out, even though there is no particular reason to. I guess another fix besides running DCE would be to make MNewDerivedTypedObject safe for parallel execution (which should be possible, I think).
Comment on attachment 8391785 [details] [diff] [review]
Bug983987.diff

Review of attachment 8391785 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, but to be clear: these MNewDerivedTypedObject have no uses but are in the entry basic block? Otherwise they shouldn't cause the analysis to reject the script, right?
Attachment #8391785 - Flags: review?(shu) → review+
(In reply to Niko Matsakis [:nmatsakis] from comment #3)
> Shu: Sometimes, but the main point is that they lie along code paths that
> will be taken. So when the execution gets there, we bail out, even though
> there is no particular reason to. I guess another fix besides running DCE
> would be to make MNewDerivedTypedObject safe for parallel execution (which
> should be possible, I think).

I don't understand. We still run DCE *after* the analysis, why should we bail out if they have no uses?
Attached patch Bug983987.diffSplinter Review
After thinking on it, I decided a more limited fix is in order, since the *right* fix is really to create parallel safe paths for NewDerivedTypedObject anyhow (bug 984090) -- but I don't feel like doing that right now both because it's Sunday and because I'm expecting to make some changes both to catch up with spec and to make allocating typed objects lighter and more efficient, and I don't precisely know how those changes would affect any jitted code we'd write.
Attachment #8391785 - Attachment is obsolete: true
Attachment #8391840 - Flags: review?(shu)
Attachment #8391840 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/f3d1ea98d8d5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: