603779 - JM: Refactor ICs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: swap derivation of assemblers

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2: helper for dense array loads

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3: abstract clasp loads

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4: refactor StateRemat

part of this patch fixes dead state in PICInfo and misuse of pic.kind - SETMETHOD looks initialized completely wrong.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5: refactor ValueRemat

adds a bunch of accessors to ValueRemat so we don't get wonders like pic.u.vr.u.s.type.reg

Comment 6

[David Mandelin [: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.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Nope, not sure why AboveOrEqual was there. Fixed these, thanks.

Comment 8

[Sean Stangl [:sstangl]]

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.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6: extract common structure info

Starting to pave the way to separate ICs into separate structs. I'll hold that off until after GETELEM is done, though.

Comment 10

[David Mandelin [: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;

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/0c5ec2e90378

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Somehow this busted
http://hg.mozilla.org/tracemonkey/rev/c835d30c91cf

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 15

[Paul Wright]

Re-landing?

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/24fb83f7a0da

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In the feature, please don't fold multiple patches into one commit.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/24fb83f7a0da

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.