nanojit: avoid getresv() because it has a redundant test

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Created attachment 398068 [details] [diff] [review]
patch

We see this pattern a lot in the Assembler:

    Reservation* resv = getresv(ins);
    NanoAssert(resv);
    ... use resv->reg, etc ...

getresv() is inlined, so it becomes this:

    Reservation* resv = ins->resv();
    resv = (r->used ? r : 0);
    NanoAssert(resv);
    ... use resv->reg, etc ...

In optimised builds the assert is removed:

    Reservation* resv = ins->resv();
    resv = (r->used ? r : 0);
    ... use resv->reg, etc ...

The r->used test should always succeed (if it doesn't, there's a bug which
a debug build should catch).

This patch removes all occurrences of this idiom in Assembler.cpp and
Nativei386.cpp (but not the other back-ends), replacing them with this:

    Reservation* resv = ins->resv(ins);
    NanoAssert(resv->used);
    ... use resv->reg, etc ...

thus avoiding the unnecessary test.  (A couple of the modified places didn't have the assertion, but the code relied on resv->used being true.)

Furthermore, I introduced resvUsed() which does the assert itself, so now 
you actually see this:

    Reservation* resv = ins->resvUsed(ins);
    ... use resv->reg, etc ...

I also changed other "resv = getresv(ins)... if (resv)" occurrences to 
"resv = ins->resv()... if (resv->used)" for consistency and readability.

I've run SS several times and am seeing a 0--3ms speedup.  I also checked 
the generated asm and it's several instructions fewer for each occurrence 
and so is a clear win in terms of code size.
Attachment #398068 - Flags: review?(gal)

Comment 1

9 years ago
Looks great, but I am not a big fan of the resvUsed() name. I think I prefer the explicit assert. It helps reading the code if its explicit there, at least for me. If you find a better name for resvUsed, I would be ok with that too. Any ideas?
(Assignee)

Comment 2

9 years ago
Earlier I was using resv() for the checking version, and resvMaybeUnused() for the unchecked version -- is that any better?

Or if you want more verbosity: getUsedResv()?

Comment 3

9 years ago
getUsedResv sounds pretty cheesy too. I don't have any great ideas either though. resvOrNull? I still like resv() and explicit asserts best.
(Assignee)

Comment 4

9 years ago
resvOrNull() matches the old behaviour -- resvOrAssert() would be more appropriate.

The reason I want to remove the assertion is that if you look through Assembler.cpp and the back-ends there is an amazing amount of code dealing with reservations when really a reservation is a middle-man -- what we're really interested in is the reg/used/arIndex fields.  I want to reduce some of this noise, it obscures the interesting work done by the code.  Maybe functions like ins->getReg() and ins->setReg() would be better.

But for the moment I'll probably just put the asserts back in.
URL:

Comment 5

9 years ago
Yeah, I agree. getresv() is a leftover of when there was a mapping table in between. Maybe we should get rid of the Reservation struct.

Updated

9 years ago
Attachment #398068 - Flags: review?(gal) → review+

Comment 6

9 years ago
Comment on attachment 398068 [details] [diff] [review]
patch

Looks good. Handle resv*() as you see fit. All options are about equally ugly.
(Assignee)

Comment 7

9 years ago
http://hg.mozilla.org/tracemonkey/rev/4911291fc158
Whiteboard: fixed-in-tracemonkey

Updated

9 years ago
Blocks: 515548
http://hg.mozilla.org/mozilla-central/rev/4911291fc158
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 518493

Comment 9

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9f63d8565bff
status1.9.2: --- → beta1-fixed
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.