Closed Bug 538924 Opened 10 years ago Closed 10 years ago

nanojit: rework reservations

Categories

(Core Graveyard :: Nanojit, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(1 file, 2 obsolete files)

Attached patch patach (obsolete) — Splinter Review
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.
Attachment #421005 - Flags: review?(edwsmith)
From a quick skim, I like this a lot -- makes parsing the code intention much easier.
"IsInFrame" might be better than "IsInAr"? Just a micro nit.
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.
Attachment #421005 - Flags: review?(edwsmith) → review+
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #421005 - Attachment is obsolete: true
Attachment #421587 - Flags: review?(edwsmith)
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.
Attachment #421587 - Attachment is patch: true
Attachment #421587 - Attachment mime type: application/octet-stream → text/plain
Attached patch patch v3Splinter Review
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).
Attachment #421587 - Attachment is obsolete: true
Attachment #421708 - Flags: review?(edwsmith)
Attachment #421587 - Flags: review?(edwsmith)
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.)
One could argue that deprecated_oldFunction() deserves to be horror-inducing...
Touché!
Yeah, ickiness was the intent.  And I didn't want to have to capitalize the first letter, eg. getReg -> deprecatedGetReg().
Attachment #421708 - Flags: review?(edwsmith) → review+
Whiteboard: fixed-in-nanojit
http://hg.mozilla.org/tracemonkey/rev/81db61bdcd3f
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
http://hg.mozilla.org/mozilla-central/rev/81db61bdcd3f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.