538924 - nanojit: rework reservations

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: patach

This patch reworks how LIns regs/arIndexes are stored during code
generation.  Only the i386 back-end is done so far, I'll do the others
if/when people are happy with the overall approach.

- Previously we had three fields: 'used', 'reg', 'arIndex'.  'used'
  indicated if 'reg' or 'arIndex' might be in use.  Then, 'UnknownReg' was
  the special non-value for 'reg' and 0 was the special non-value for
  'arIndex'.

  Now 'used' is gone, and we have 'inReg' and 'inArIndex' which indicate if
  the 'reg' field and 'arIndex' fields are in use, respectively.  This means
  that 'UnknownReg' (which really meant "in a spill slot" in most cases) can
  be dispensed with, and 0 can be used as an arIndex (although I haven't
  yet changed things so that it is, I didn't want to fiddle with the
  AR-reservation stuff in this patch).
  
  In other words, we are (and always have been) dealing with two "maybe"
  values:  a maybe-register, and a maybe-AR-slot.  The maybe-ness of each
  value is now encoded explicitly via distinct tag bits rather than via an
  awkward combination of a shared tag bit and two special non-values.

  One consequence is that 'arIndex' is reduced from 16 bits to 15, but that
  shouldn't matter since we aren't even close to using all the bits.

- Renamed/refactored all the reservation manipulation functions accordingly.
  They're easier to understand now, occasionally drastically so (eg.
  see findRegFor2()).

- Renamed nanojit::disp() as arDisp() to distinguish it clearly from
  LIns::disp().

- There are a couple of places in the i386 back-end where being able to
  specify "no register" is useful.  Now using 'UnspecifiedReg' instead of
  'UnknownReg' for this.

- I also reworked asm_spill() and removed asm_spilli().  It's much nicer now.

Overall it doesn't change codegen, just makes everything a bit easier to
understand -- we're dealing with registers and AR slots, they are clearly
distinct, and we don't have to worry about special values such as
UnknownReg.  Given how complicated codegen is and how often these
reservation values are manipulated, IMHO this is a clear win for clarity.
It also ends up as slightly fewer instructions overall.

Comment 1

[Steven Johnson]

From a quick skim, I like this a lot -- makes parsing the code intention much easier.

Comment 2

[Andreas Gal :gal]

"IsInFrame" might be better than "IsInAr"? Just a micro nit.

Comment 3

[Edwin Smith]

Note that arIndex(0) won't be usable unless we also build in a constant offset, since EBP-0 itself points to the end of the stack frame:

   [valid area of the stack]
                           ^EBP

Of course, I'd be in favor general refactoring, or inserting an assumed offset, to actually enable using the full range of arIndex.

Comment 4

[Nicholas Nethercote [:njn] (On Long Service Leave)]

(In reply to comment #2)
> "IsInFrame" might be better than "IsInAr"? Just a micro nit.

I don't particularly like "AR", but as long as we're using it I'd like to use it consistently.  Having two different names for the same thing isn't good.

Comment 5

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: patch v2

This patch reworks things properly for the i386 and X64 back-ends.  The other back-ends are harder for me to do without access to a dev machine, and also this reworking is easier once the regstate fixes are done (see bug 535707, bug 535708, bug 535709).  So I've left them.

But that required leaving part of the old reservation interface around.  The easy changes I've done in ARM/PPC/Sparc, but for the harder ones I've marked those old functions with a "_Old" suffix to indicate that they are deprecated.  This seems like a reasonable compromise.

Comment 6

[Jeff Walden [:Waldo]]

Not to nitpick too much, but it seems to me that using the full word "deprecated" as the prefix would be slightly clearer and would introduce slightly more typing pain for anyone who wanted to use it.  It's what WebKit uses for marking old methods as slowly dying as I understand it, and I happen to think it's a slightly better choice.

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: patch v3

I changed the "_Old" suffixes to "deprecated_" prefixes as per Waldo's suggestion.  I also added such prefixes to freeRsrcOf() and prepResultReg(), which were deprecated elsewhere (bug 516347).

Comment 8

[Jeff Walden [:Waldo]]

Okay, now I really really am nitpicking, but I wouldn't mix camelCaps with underscores like that.  :-)  Exaggerating slightly, but I think the only precedent for that is wallet code.  (Ask some old Mozilla hand about wallet and its naming conventions, and prepare to weep in horror.)

Comment 9

[Steven Johnson]

One could argue that deprecated_oldFunction() deserves to be horror-inducing...

Comment 10

[Jeff Walden [:Waldo]]

Touché!

Comment 11

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Yeah, ickiness was the intent.  And I didn't want to have to capitalize the first letter, eg. getReg -> deprecatedGetReg().

Comment 12

[Nicholas Nethercote [:njn] (On Long Service Leave)]

ping?

Comment 13

[Nicholas Nethercote [:njn] (On Long Service Leave)]

http://hg.mozilla.org/projects/nanojit-central/rev/51a78a175b10

Comment 14

[Nicholas Nethercote [:njn] (On Long Service Leave)]

http://hg.mozilla.org/tracemonkey/rev/81db61bdcd3f

Comment 15

[Steven Johnson]

http://hg.mozilla.org/tamarin-redux/rev/f2451731d8a2

Comment 16

[Nicholas Nethercote [:njn] (On Long Service Leave)]

http://hg.mozilla.org/mozilla-central/rev/81db61bdcd3f