514349 - nanojit: start to kill Reservations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

Reservations used to be needed when there was a table of them, but now that every instruction has one they just get in the way.  We see this kind of thing a lot:

  Reservation* resv = ins->resvUsed();
  Register r = resv->reg;
  ... do stuff with r...
  resv->reg = UnknownReg;

'resv' is just an unnecessary middle-man here.
 
This patch begins the process of removing them.  I say "begin" because it only updates Assembler.cpp and Nativei386.cpp;  the other back-ends are difficult/impossible for me to convert because I can't test them.  (I could try generating patches for them and get someone else to fix any compilation errors and test them, if that's useful.)

Each instruction still has a Reservation, for the moment.  But I've added an API for accessing the fields in it directly via the parent instruction, without using an intermediate Reservation, eg. getReg(), setReg().  This API also includes some functions for common operations, eg. isUsed(), isKnown(), and isUnusedOrUnknown() (which occurs surprisingly often).  The end result is that the code above now looks like this:

  Register r = ins->getReg();
  ... do stuff with r...
  ins->setReg() = UnknownReg;

This is more compact, and means you don't have to think about the Reservation acting as a middle-man -- it's clearer that the register belongs directly to the instruction.

Once all the back-ends have been changed, then the Reservation type can be removed.  We could also then make the fields public and get rid of getReg()/setReg() if we wanted;  we can't do that now so long as Reservation still persists inside LIns.

The attached patch has no measurable performance effect.  It involves slightly longer access chains (eg. ins->lastWord->reg) but removes a lot of ins->usedResv() calls.  These changes seem to cancel out, as I looked at the generated code for several affected functions and it was, surprisingly enough, unchanged on both Linux and Mac.  Once Reservation is removed entirely and the access chains are shorter (eg. ins->reg) it might even provide a slight speed-up, although it seems GCC is pretty good at optimising field accesses.  The real benefit is that it makes the code shorter and easier to read.

Ed, I've asked you for a review but I've done that a lot lately;  feel free to pass it on to anyone else you think appropriate.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

- Rename ins->isUnusedOrUnknown() to ins->isUnusedOrHasUnknownReg().

- Rename ins->isKnown to ins->hasKnownReg()

- Introduce isKnownReg() which is sometimes nicer to use that ins->hasKnownReg().
  It also avoids double negatives like "r != UnknownReg".

Comment 2

[Edwin Smith]

Comment on attachment 398322 [details] [diff] [review]
patch, v2

nitty naming suggestions offered from the peanut gallery:
   LIns.hasKnownReg() -> LIns.hasReg()
   isKnownReg(Register) -> isValid(Register) ?  (maybe "valid" is one adjective too many for this nomenclature...)

Assembler.cpp:282 - it is enough to assert(r2 == r) here.

adding Rick for another set of eyes since many lines of fragile code had to be touched.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> (From update of attachment 398322 [details] [diff] [review])
> nitty naming suggestions offered from the peanut gallery:
>    LIns.hasKnownReg() -> LIns.hasReg()
>    isKnownReg(Register) -> isValid(Register) ?  (maybe "valid" is one adjective
> too many for this nomenclature...)

I don't like "valid" much, too vague.

hasKnownReg() and isKnownReg() deliberately have similar names because they are doing almost the same thing, and they deliberately echo "UnknownReg".  So I'm reluctant to change them.

However, I have wondered if "UnknownReg" is the best name... perhaps "NoReg" or "NotInReg" or "Spilled" would be better?

> Assembler.cpp:282 - it is enough to assert(r2 == r) here.

Yes, I've made that change in my invariants patch (bug 512634).

Comment 4

[Rick Reitmaier]

Comment on attachment 398322 [details] [diff] [review]
patch, v2

nice

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/cbb2bdb22a22

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/cbb2bdb22a22

Comment 7

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ad18976bfae4