IonMonkey: don't generate unecessary phi instructions

RESOLVED FIXED

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rpearl, Assigned: rpearl)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 550268 [details]
test case

If we insert a phi which merges two copy instructions, one of which is inside a loop and carried to the phi from a back edge, then, once we copy-propagate, we have phi instructions where neither input is from the loop.

This causes issues with GVN, since it cannot reach a fixed point--changing the value number of such a phi necessarily causes a consumer of the instruction to change, so there is a sequence of dependencies which can cause GVN to never reach a fixed point.

Options for fixing (discussed in person) include copy-propagating after GVN, special casing phis and cleaning them up, or not generating them in the first place.

The attached test-case causes GVN to infloop.
(Assignee)

Comment 1

7 years ago
Created attachment 550275 [details] [diff] [review]
patch v0

In this patch, we do not generate redundant phis.
Attachment #550275 - Flags: review?(dvander)
(Assignee)

Updated

7 years ago
Attachment #550275 - Flags: feedback?(sstangl)
(Assignee)

Comment 2

7 years ago
Created attachment 550279 [details] [diff] [review]
patch v1

Sorry, that patch had little bit of very incorrect cruft!
Attachment #550275 - Attachment is obsolete: true
Attachment #550275 - Flags: review?(dvander)
Attachment #550275 - Flags: feedback?(sstangl)
Attachment #550279 - Flags: review?(dvander)
Attachment #550279 - Flags: feedback?(sstangl)
Comment on attachment 550279 [details] [diff] [review]
patch v1

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

::: js/src/ion/MIRGraph.cpp
@@ +521,5 @@
> +        // So long as we keep local variables separate, and not remove copies
> +        // until after SSA building is complete, then we need not insert phis
> +        // if the entry definition is just a copy of the exit definition.
> +        // If at any point, one local variable is treated in some way that is
> +        // no a copy of the other, then the exit definition will also differ.

What do "separate" and "treated" mean here?

@@ +542,5 @@
> +        if (entryDef->isCopy())
> +            entryDef = entryDef->getOperand(0);
> +
> +        if (exitDef->isCopy())
> +            exitDef = exitDef->getOperand(0);

I'm convinced that it's safe to drop the phi, but this will change semantics in another subtle way: if the inputs are *not* the same, you've just unwrapped the copies, and will now replace uses of the contained name, not the copy. I don't think that's safe.
Attachment #550279 - Flags: review?(dvander)
(Assignee)

Comment 4

7 years ago
Created attachment 550762 [details] [diff] [review]
patch v2

Follow copies when determining whether to place a phi, but don't update the entry and exit definitions, so that if we do actually insert a phi, it keeps copies distinct.
Attachment #550279 - Attachment is obsolete: true
Attachment #550279 - Flags: feedback?(sstangl)
Attachment #550762 - Flags: review?(dvander)
Comment on attachment 550762 [details] [diff] [review]
patch v2

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

Looks good, thank you for the detailed comment!

::: js/src/ion/MIRGraph.cpp
@@ +1,1 @@
> +const /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-

what

@@ +502,5 @@
>  #endif
>  }
>  
> +static inline MDefinition *
> +FollowCopy(MDefinition *def) {

{ on newline
Attachment #550762 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

7 years ago
I blame my mouse magically pasting. With nits addressed,
http://hg.mozilla.org/projects/ionmonkey/rev/a459db11a653
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.