[JITs] Unify InIRGenerator and HasOwnIRGenerator

RESOLVED FIXED

Status

()

Core
JavaScript Engine: JIT
P3
normal
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

({leave-open})

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

8 months ago
We should combine the InIRGenerator (bug 1334187) and HasOwnIRGenerator (bug 1344469) since they are very similar. This should bring them up to feature parity.
(Assignee)

Updated

8 months ago
Blocks: 1337773
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Assignee: nobody → tcampbell
(Assignee)

Comment 5

7 months ago
Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cbf0ace32ac95221640e91882ec42b7eb176d9c

(The mozreview one somehow broke Gecko Decision Task..)

Comment 6

7 months ago
mozreview-review
Comment on attachment 8863384 [details]
Bug 1359952 - Fix CacheIRCompiler handling of boolean results

https://reviewboard.mozilla.org/r/135144/#review138456
Attachment #8863384 - Flags: review?(jdemooij) → review+

Comment 7

7 months ago
mozreview-review
Comment on attachment 8863385 [details]
Bug 1359952 - Remove shape arg from TestMatchingReceiver

https://reviewboard.mozilla.org/r/135146/#review138458

Nice.
Attachment #8863385 - Flags: review?(jdemooij) → review+

Comment 8

7 months ago
mozreview-review
Comment on attachment 8863386 [details]
Bug 1359952 - Add ownProp flag to CanAttachDenseElementHole

https://reviewboard.mozilla.org/r/135148/#review138460
Attachment #8863386 - Flags: review?(jdemooij) → review+

Comment 9

7 months ago
mozreview-review
Comment on attachment 8863387 [details]
Bug 1359952 - Add HasPropIRGenerator

https://reviewboard.mozilla.org/r/135150/#review138462

Thanks, very nice!

::: js/src/jit/CacheIR.cpp:443
(Diff revision 1)
>                  writer.guardShape(protoId, proto->as<NativeObject>().lastProperty());
>                  proto = proto->staticPrototype();
>                  lastObjId = protoId;
>              }
>          }
> -    } else if (obj->is<UnboxedPlainObject>()) {
> +    } else if (obj->is<UnboxedPlainObject>() && expandoId.isSome()) {

I think I'd prefer adding an expicit HasPropIRGenerator::tryAttachUnboxed and using that instead of tryAttachNative.. Then we don't need this change right? Follow-up patch is fine.

::: js/src/jit/CacheIR.cpp:2095
(Diff revision 1)
> -    if (mode_ == ICState::Mode::Megamorphic) {
> +        // For HasOwn, we don't need to guard prototypes. Set holder to object
> +        // even though property is not found to avoid guarding the protochain.
> +        holder = obj;

It may be more explicit to do this:

if (hasOwn) {
    TestMatchingReceiver...
} else {
    EmitReadSlotGuard...
}

::: js/src/jit/CacheIR.cpp:2158
(Diff revision 1)
>      RootedObject obj(cx_, &val_.toObject());
> -
>      ObjOperandId objId = writer.guardIsObject(valId);
>  
> -    if (tryAttachProxyElement(keyId, obj, objId))
> +    // Optimize DOM Proxies for JSOP_HASOWN
> +    if (cacheKind_ == CacheKind::HasOwn)

Nit: add {} to the outer if-statement. Also please file a follow-up bug to support the megamorphic and proxy cases for JSOP_IN, maybe we can use templates for the VM functions.
Attachment #8863387 - Flags: review?(jdemooij) → review+
Comment on attachment 8863387 [details]
Bug 1359952 - Add HasPropIRGenerator

Maybe evilpie should look at this too in case I missed something.
Attachment #8863387 - Flags: feedback?(evilpies)
Comment on attachment 8863387 [details]
Bug 1359952 - Add HasPropIRGenerator

Looks good. I remember there was some problem with EmitReadSlotGuard for HasOwn, that's probably the expandoId you did.
Attachment #8863387 - Flags: feedback?(evilpies) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

7 months ago
TODO: Refactor the UnboxedObject / TypedObject cases to be more explicit / documented. They should work as is now, but it is not obvious that the special concerns are not handled. Also remove unnecessary guards described in https://bugzilla.mozilla.org/show_bug.cgi?id=1344469#c38.
Keywords: leave-open
(Assignee)

Updated

7 months ago
Blocks: 1361731
(Assignee)

Updated

7 months ago
Blocks: 1361735
Comment hidden (mozreview-request)

Comment 18

7 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/522b7ef5bd11
Fix CacheIRCompiler handling of boolean results r=jandem
https://hg.mozilla.org/integration/autoland/rev/b33d51a80290
Remove shape arg from TestMatchingReceiver r=jandem
https://hg.mozilla.org/integration/autoland/rev/a6ce36227095
Add ownProp flag to CanAttachDenseElementHole r=jandem
https://hg.mozilla.org/integration/autoland/rev/c06620d52d5f
Add HasPropIRGenerator r=jandem

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/522b7ef5bd11
https://hg.mozilla.org/mozilla-central/rev/b33d51a80290
https://hg.mozilla.org/mozilla-central/rev/a6ce36227095
https://hg.mozilla.org/mozilla-central/rev/c06620d52d5f
Depends on: 1362424
No longer depends on: 1362424
(Assignee)

Updated

3 months ago
Priority: -- → P3
(Assignee)

Comment 20

3 months ago
Closing bug as follow-up is now part of Bug 1357759.
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.