1176240 - MFilterTypeSet add extra register pressure for duplicating unused values.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

MFilterTypeSet is introduced as a way to add additional type information to the condition expression, but once back at the join block, it introduces a new Phi which has the same type set as the MTest argument.

If I think we should fold MPhi(MFilterTypeSet(X), MFilterTypeSet(X), …) to X, iff the MPhi post-dominate the MTest.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: WIP

This will try to fold MPhi with combination of MFilterTypeSet(x) and/or X

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8659429 [details] [diff] [review]
WIP

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

Thanks for looking at this issue :)

::: js/src/jit/MIR.cpp
@@ +1733,5 @@
> +        return nullptr;
> +
> +    // Subject is better typed (according to typeset). Don't fold.
> +    if (subject->resultTypeSet() && resultTypeSet()) {
> +        if (!subject->resultTypeSet()->isSubset(resultTypeSet()))

It sounds to me that the above condition will always hold.

The issue is that the union of the type-sets does not refine the type of the MFilterTypeSet operand.

I think what you should look for, is to walk the MFilterTypeSet chains until you reach an operand which is in a block which is dominating the current Phi.  Then you want to know if this input type of this operand got refined by the MFilterTypeSet, and if it is not, then we should replace this MFilterTypeSet by this definition in the Phi operands.

Then by applying the operandIfRedundant optim, we can get rid of the case where none of the FilterTypeSet adding any information.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8659429 [details] [diff] [review]
WIP

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

::: js/src/jit/MIR.cpp
@@ +1728,5 @@
> +    if (subject->type() != type())
> +        return nullptr;
> +
> +    // Subject is better typed (according to type). Don't fold.
> +    if (subject->resultTypeSet() && !resultTypeSet())

Woops this should be:

// Phi is better typed (according to type). Don't fold.
if (resultTypeSet() && !subject->resultTypeSet())

@@ +1733,5 @@
> +        return nullptr;
> +
> +    // Subject is better typed (according to typeset). Don't fold.
> +    if (subject->resultTypeSet() && resultTypeSet()) {
> +        if (!subject->resultTypeSet()->isSubset(resultTypeSet()))

Woops, comment is wrong. code is correct:
// Phi is better typed (according to typeset). Don't fold.

That should also fix your concern and is faster/easier than walking the chain and find if a block is dominating etc ... The MPhi should get folded when subject has better or equal type to the MPhi. Where in most cases it will be equal and get folded.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8659429 [details] [diff] [review]
WIP

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

Apply your comments and r=me ;)

::: js/src/jit/MIR.cpp
@@ +1721,5 @@
> +        return nullptr;
> +
> +    MDefinition* subject = getOperand(0);
> +    if (subject->isFilterTypeSet())
> +        subject = subject->toFilterTypeSet()->input();

Somehow I missed this line last time I read this code. :/
So this sounds good to me.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/480c60fad352

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/480c60fad352