1059364 - Don't emit ObjectGroupDispatch fallback path if possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Probably the hottest function in Octane-deltablue is Plan.prototype.execute. We use polymorphic inlining with MTypeObjectDispatch for the c.execute() call, but unfortunately IonBuilder always adds a fallback path that has a GetPropertyCache and CallGeneric and this blocks LICM and other optimizations from optimizing the rest of the code.

It's tricky to fix because for the c.execute() call, c's TypeSet contains 5 TypeObjects but in practice we only see 3, so somehow we need to narrow it down to those 3 and then emit MTypeObjectDispatch without the fallback path.

Based on some local hacks this could win at least a few thousand points. Unfortunately the polymorphic inlining code is really complicated, so I've no idea if this is correct.

Comment 1

[Jan de Mooij [:jandem]]

Taking this for now as there are a few things I'd like to try.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: WIP

A hackish WIP patch that wins about 10% on deltablue on 32-bit. I don't see any difference with a 64-bit build though because type information is different somehow.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

With this patch we don't emit a fallback path if the incoming object types are all handled by the ObjectGroupDispatch instruction. This improves alias analysis: the fallback path if present blocks LICM/GVN. It should also help regalloc.

This wins at least 1000 points or so when I run deltablue locally, both with and without --unboxed-objects.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 8571519 [details] [diff] [review]
Patch

Review of attachment 8571519 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +5477,5 @@
> +                    MOZ_ASSERT(barrier->type() == MIRType_Object);
> +                    MOZ_ASSERT(barrier->input()->isGetPropertyCache());
> +                    MOZ_ASSERT(barrier->input()->toGetPropertyCache() == maybeCache);
> +
> +                    barrier->block()->discard(barrier);

I don't understand why it's ok to remove this barrier.  We are removing the fallback path if the dispatch handles all incoming objects, but those objects are produced by the barrier instruction and the barrier is in place because it is possible to read other objects.  Is there another place where we'll ensure that other incoming objects are caught and trigger a bailout?

@@ +5479,5 @@
> +                    MOZ_ASSERT(barrier->input()->toGetPropertyCache() == maybeCache);
> +
> +                    barrier->block()->discard(barrier);
> +                    MOZ_ASSERT(!maybeCache->hasUses());
> +                    maybeCache->block()->discard(maybeCache);

The last two lines here can be commoned with the end of the |if| block and moved after the |else| block.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> I don't understand why it's ok to remove this barrier.  We are removing the
> fallback path if the dispatch handles all incoming objects, but those
> objects are produced by the barrier instruction and the barrier is in place
> because it is possible to read other objects.

We have:

objectDef -> MGetPropertyCache -> MTypeBarrier
objectDef -> MObjectGroupDispatch

The patch is checking whether objectDef's types are all handled by the ObjectGroupDispatch. The GetPropertyCache and TypeBarrier are unrelated to this; right now we move them to the fallback path but if we don't have a fallback path we can eliminate them.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8571519 [details] [diff] [review]
Patch

See comment 5.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8571519 [details] [diff] [review]
Patch

Review of attachment 8571519 [details] [diff] [review]:
-----------------------------------------------------------------

OK, but can you add a comment describing why it's OK to remove the type barrier?  Just because the type barrier doesn't feed into the dispatch doesn't mean it's required for correctness (i.e. it's a guard).  But something like:

// The object group dispatch handles all possible incoming objects, so the cache and barrier will not be reached and can be eliminated.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57ddae8223f9

Added the comment.

Comment 9

[Guilherme Lima]

Deltablue on AWFY:

Mac 32-bit machine:
without unboxed-objects - 8% regression
with unboxed objects - 8% improvement

Mac 64-bit machine:
on both cases - 3% improvement

Windows 32-bit (browser) - no difference

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57ddae8223f9