Closed Bug 603779 Opened 12 years ago Closed 12 years ago

JM: Refactor ICs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(6 files)

While working on bug 602641 I encountered a lot of internal API that could be much cleaner, and uncovered some bugs and inefficiencies in the process. Filing this bug to collect the incremental changes in one place.
part of this patch fixes dead state in PICInfo and misuse of pic.kind - SETMETHOD looks initialized completely wrong.
Attachment #482686 - Flags: review?(dmandelin)
adds a bunch of accessors to ValueRemat so we don't get wonders like pic.u.vr.u.s.type.reg
Attachment #482687 - Flags: review?(dmandelin)
Comment on attachment 482684 [details] [diff] [review]
part 2: helper for dense array loads

This wants a little comment saying what it's for. Maybe:

// Represents an int32 property name in generated code, which
// must be either a register or a constant value.
>+struct Int32Key {

It would be nice to have all the parameters documented, but it was easy enough
to figure this one out, so only if you think it's better too.

>+    // Load a jsval from an array slot, given a key. |objReg| is clobbered.

But this is redundant with the assertion, so it can be cut.

>+    // Note: typeReg must not equal objReg.
>+    FastArrayLoadFails fastArrayLoad(RegisterID objReg, const Int32Key &key,
>+                                     RegisterID typeReg, RegisterID dataReg) {
>+        JS_ASSERT(objReg != typeReg);

This might read a hair easier if AboveOrEqual is used in both places. Is
it slower or something?

>+        // Check that the id is within range.
>+        if (key.isConstant()) {
>+            JS_ASSERT(key.index() >= 0);
>+            fails.rangeCheck = branch32(LessThanOrEqual, payloadOf(capacity), Imm32(key.index()));
>+        } else {
>+            fails.rangeCheck = branch32(AboveOrEqual, key.reg(), payloadOf(capacity));
>+        }

r+ with updated comments.
Attachment #482684 - Flags: review?(dmandelin) → review+
Attachment #482685 - Flags: review?(dmandelin) → review+
Attachment #482686 - Flags: review?(dmandelin) → review+
Attachment #482687 - Flags: review?(dmandelin) → review+
Nope, not sure why AboveOrEqual was there. Fixed these, thanks.
Comment on attachment 482682 [details] [diff] [review]
part 1: swap derivation of assemblers

Keeping the filename 'BaseAssembler.h' is a bit dubious. I still like 'SumboxAssembler', but an un-punny name would be fine too.
Attachment #482682 - Flags: review?(sstangl) → review+
Starting to pave the way to separate ICs into separate structs. I'll hold that off until after GETELEM is done, though.
Attachment #483217 - Flags: review?(dmandelin)
Comment on attachment 483217 [details] [diff] [review]
part 6: extract common structure info

I only saw one issue, in an old comment: I think the part after the comma is now lying. 

+    // Return address of slow path call, as an offset from slowPathStart.
+    JSC::CodeLocationCall slowPathCall;
Attachment #483217 - Flags: review?(dmandelin) → review+
Somehow this busted
http://hg.mozilla.org/tracemonkey/rev/c835d30c91cf
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/tracemonkey/rev/45805d1b90d4

Urgh, templates. BaseAssembler.h has something like:

  template <typename T>
  loadPayload(T address, RegisterID reg)

I accidentally typed "loadPayload(reg1, reg2)", when I meant "move(reg1, reg2)". C++ implicitly created an Address(reg1, 0) and did a memory load. Somehow, this didn't crash anywhere in ref or trace-tests.

Luke tells me C++0x Concepts would have helped here.
Whiteboard: fixed-in-tracemonkey
Backed out again. A signed comparison in the array capacity check should have been unsigned. Doing a full mochitest run locally to make sure.

On the positive side, looks like this was about a 1% win. Plausible, since a few IC bugs were fixed and pinEntry() does better register allocation now.
Whiteboard: fixed-in-tracemonkey
Re-landing?
In the feature, please don't fold multiple patches into one commit.
http://hg.mozilla.org/mozilla-central/rev/24fb83f7a0da
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
It's one patch, separated for ease of review. If you don't like this workflow, or if I've missed that it's somehow against policy or practice, I'd be happy to hear about it offline.
You need to log in before you can comment on or make changes to this bug.