538000 - Generating prolog and method body into different buffers could enable simple expression hoisting

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

Attachment: First attempt, does the hoisting, passes regressions, somewhat hacky

currently we generate the whole method in one linear pass, into one buffer, then read that one buffer back to generate code.

We could generate concurrently into two bufers (prolog, body), then sequentially read them back.  The pipeline nature of code generation should make this relatively simple.

This enables generating expressions like env->vtable, env->vtable->scope, etc, in the prolog, but only once they're found to be needed while generating the body.

Comment 1

[Steven Johnson]

I take it the "0 &&" in Assembler.cpp is just an accidental inclusion?

Comment 2

[Edwin Smith]

(In reply to comment #1)
> I take it the "0 &&" in Assembler.cpp is just an accidental inclusion?

correct.

Comment 3

[Edwin Smith]

Attachment: (v2) second attempt, cleaned up somewhat

Remaining cleanup:  verbose output from the LIR generation pipeline does not show code being emitted to the prolog while we're emitting code into the body.  (but it does show the initial prolog code).

needs general inspection of the verbose output for clarity 

needs code review to make sure its obvious what is happening

need to refactor nanojit::ReverseLister so we can reuse it instead of copying it into CodegenLIR.cpp (that's a hack!)

Comment 4

[Edwin Smith]

Attachment: Modify nanojit::live() so caller can supply the LirFilter to use

Comment 5

[Steven Johnson]

Attachment: Patch v3, further cleanup

Cleaned up a bit further. Prolog code is now output inline with a prefix (but not distinguished in final listing).

Comment 6

[Steven Johnson]

Attachment: Modify nanojit::live, expose ReverseLister, add some options to VerboseWriter

Comment 7

[Rick Reitmaier]

Is this going to be our general approach to handling code movement and more general basic blocks optimization?   That is, putting code is different buffers?
  
If so, them maybe we should look at writing the code in a more general fashion, not specific for prologue.  For example, use an array of LirBuffs (in this case we're using 2).

Also, previously our buffers have been self-contained.  In this case, we now have references from one buffer to another.  Should we have a data structure which represents this fact?

Comment 8

[Edwin Smith]

(In reply to comment #7)
> Is this going to be our general approach to handling code movement and more
> general basic blocks optimization?   That is, putting code is different
> buffers?

I hope not; LirBuffers are just meant to capture the output of a LirWriter pipline (a linear stream of instructions).

> If so, them maybe we should look at writing the code in a more general fashion,
> not specific for prologue.  For example, use an array of LirBuffs (in this case
> we're using 2).
> 
> Also, previously our buffers have been self-contained.  In this case, we now
> have references from one buffer to another.  Should we have a data structure
> which represents this fact?

nanojit has always supported refs from one buffer to another; trace tree branches worked that way.  For example, expressions in a branch can point to expressions in the trunk.

Maybe the container-for-multiple-lirbufs is Fragment, but IMO its premature to stuff what we're doing into Fragment.  I'm happy with a point solution for now.

Comment 9

[Edwin Smith]

Comment on attachment 420385 [details] [diff] [review]
Modify nanojit::live, expose ReverseLister, add some options to VerboseWriter

note the comment on ReverseLister was slightly incorrect before, and now: the listing is printed when finish() is called, not when ReverseLister is destructed.

Comment 10

[Edwin Smith]

Comment on attachment 420384 [details] [diff] [review]
Patch v3, further cleanup

generally it's okay but it looks like something has gone haywire with specially assigned expression names.  e.g. the LIR_alloc called "vars" is now showing as "ialloc2".

The intention was for all names of all expressions to be attached to frag->lirbuf->names, which is what everyone uses for verbose output.  (probably, LirNameMap doesn't belong on LirBuffer, but its harmless that way).

Comment 11

[Steven Johnson]

(In reply to comment #9)
> (From update of attachment 420385 [details] [diff] [review])
> note the comment on ReverseLister was slightly incorrect before, and now: the
> listing is printed when finish() is called, not when ReverseLister is
> destructed.

Solved easily enough. Does this need a Mozilla reviewer or think it's ok to push as-is?

Comment 12

[Steven Johnson]

Attachment: Patch v4, further cleanup

Fixes the verbose problems Edwin saw:
-- uses a single LirNameMap for both prolog and body
-- don't use always_flush, instead, explicitly call flush() after writing to prolog out-of-order
-- add names to map for env_scope (etc) for cleaner output

Comment 13

[Graydon Hoare :graydon]

Comment on attachment 420385 [details] [diff] [review]
Modify nanojit::live, expose ReverseLister, add some options to VerboseWriter

Doesn't look too scary. The latter patch is a little more so though!

Comment 14

[Steven Johnson]

Comment on attachment 420385 [details] [diff] [review]
Modify nanojit::live, expose ReverseLister, add some options to VerboseWriter

nj: http://hg.mozilla.org/projects/nanojit-central/rev/e5ef97918b79

Comment 15

[Steven Johnson]

Comment on attachment 420385 [details] [diff] [review]
Modify nanojit::live, expose ReverseLister, add some options to VerboseWriter

http://hg.mozilla.org/tamarin-redux/rev/86bef836634d

Comment 16

[Steven Johnson]

Comment on attachment 420589 [details] [diff] [review]
Patch v4, further cleanup

http://hg.mozilla.org/tamarin-redux/rev/2563f2ac8987

Comment 17

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/6eeab16a494e

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6eeab16a494e

Comment 19

[Chris Peyer]

Engineering work item.  Marking verified fixed.