463531 - Need loop header instruction for hoisting

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[Rob Arnold [:robarnold]]

For future optimizations, it would be nice and efficient to perform or hoist some computations outside of the traced loop. We should generate a LIR_header instruction to mark the beginning of the loop.

Comment 1

[Andreas Gal :gal]

Attachment: v1

Comment 2

[Andreas Gal :gal]

Attachment: v2, tested

Comment 3

[Rob Arnold [:robarnold]]

This doesn't seem to affect the native code generated.

Comment 4

[Andreas Gal :gal]

Its not supposed to, except that instructions emitted between LIR_header and LIR_start will be above the loop header (which is LIR_header).

Comment 5

[Rob Arnold [:robarnold]]

When I hoist state above LIR_header, I get crashes on the 2nd iteration because it doesn't get properly unspilled on the loop edge.

Comment 6

[Andreas Gal :gal]

Can you attach a short patch that emits code in between the way you want it emitted? I will debug it a bit.

Comment 7

[Rob Arnold [:robarnold]]

Attachment: Patch queue bundle

Here's the bundle of my repo

Comment 8

[Edwin Smith]

should we introduce a new LIR structure, similar to VMSideExit, for labels? if each label could have its own structure, then they could be distinguished, and we might not need special LIR_header instruction? 

on the other hand if we have the opcodes, checking ins->opcode() can be easier than peeking into some struct.  depends on how/where it's used.

Comment 9

[Rob Arnold [:robarnold]]

Any progress on this Andreas?

Comment 10

[Andreas Gal :gal]

Attachment: refresh

Comment 11

[Andreas Gal :gal]

Attachment: refresh, without junk

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: hack to get loop-hoisted stack slots

The problem with the previous patch is that (aside from LIR_header actually doing the same thing as LIR_start), is that stack slots can be re-used across the loop and hoisted spills get clobbered.

A _really, really_ hacky fix for this was to look for live spills at the loop header, and if they've been used more than once, to reserve pinned stack space.

If there was an easy way to tell whether an LIns* was between LIR_start and LIR_header then this wouldn't be necessary.  Possibly something like TM's tracker would work.

There is still a problem in that you can't use these hoisted instructions on branch traces.  We'd need either tree recompilation or some structure capable of recovering or caching the stack slots.  In fact, right now, the mere presence of a branch trace might be able to clobber these pinned stack slots, though trace-tests does not trigger this.

The junk added in jstracer.cpp is a silly test case.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: hack to get loop-hoisted stack slots

Whoops, attached was an interdiff - here's a full patch.

Comment 14

[Edwin Smith]

Don't know the status of this but LIR_header and LIR_label seem pretty similar; what are the important differences?

Comment 15

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Edwin, is this still wanted for Nanojit?

Comment 17

[Edwin Smith]

nope, WONTFIX.