Closed Bug 1176240 Opened 5 years ago Closed 4 years ago

MFilterTypeSet add extra register pressure for duplicating unused values.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: nbp, Assigned: h4writer)

Details

Attachments

(1 file)

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.
Attached patch WIPSplinter Review
This will try to fold MPhi with combination of MFilterTypeSet(x) and/or X
Assignee: nobody → hv1989
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 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 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.
Attachment #8659429 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/480c60fad352
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.