Closed Bug 538924 Opened 10 years ago Closed 10 years ago
nanojit: rework reservations
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.
(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.
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.
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.
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).
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...
Yeah, ickiness was the intent. And I didn't want to have to capitalize the first letter, eg. getReg -> deprecatedGetReg().
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.