On Mon, Jan 29, 2018 at 11:58:45PM -0700, Jeff Law wrote:
> On 01/29/2018 04:37 PM, Jakub Jelinek wrote:
> > Hi!
> >
> > As mentioned in the PR, cunroll sometimes registers some SSA_NAMEs for
> > renaming and then invokes some SCEV using functions before finally updating
> > the SSA form. On the testcase we end up with 3 degenerate PHIs pointing at
> > each other, so follow_copies_to_constant loops forever.
> I'm at a loss to understand how we could get in that situation. Was it
> edge removal (if so for which PHI) or the duplicate process that created
> the loop?
The original loop before unrolling had following PHIs for the y parameter:
<bb 3> [local count: 118111601]:
# y_37 = PHI <y_8(11), y_37(14)>
<bb 13> [local count: 3277506]:
# y_29 = PHI <y_37(5)>
<bb 6> [local count: 3498303]:
# y_8 = PHI <y_20(D)(2), y_29(13)>
and actually no other PHIs, so effectively all uses of y in the function
could be y_20(D), but we haven't cleaned it up yet; before pre we had:
<bb 5> [local count: 118111601]:
# y_29 = PHI <0(4), y_37(3)>
which prevented optimizing everything into y_20(D).
In the cunroll emergency dump I've done when it was looping I got:
;; basic block 15, loop depth 0
;; pred: 2
# y_25 = PHI <y_20(D)(2)>
;; basic block 17, loop depth 1
;; pred: 16
;; 19
# y_10 = PHI <y_8(16), y_37(19)>
;; basic block 20, loop depth 0
;; pred: 18
# y_5 = PHI <y_37(18)>
;; basic block 3, loop depth 2
;; pred: 11
;; 14
# y_37 = PHI <y_8(11), y_37(14)>
;; basic block 13, loop depth 1
;; pred: 5
# y_29 = PHI <y_37(5)>
;; basic block 6, loop depth 1
;; pred: 20
;; 13
# y_8 = PHI <y_29(20), y_29(13)>
where bb 3, 13 and 6 are the same PHIs as before which aren't going to be
renamed, and bb20 is the new cunroll copy of bb13, bb17 copy of bb3.
The PHI results of the PHIs in the new bbs already have a new SSA_NAME, but
the PHI arguments are waiting to be renamed.
Already before the cunroll, 2 of the PHIs are degenerate and only one is
not. When SCEV is called, the only non-degenerate among those 3 previously
looks like degenerate too, but we'll be replacing y_29(20) in there with
y_5(20) during ssa update.
Now, with the patch at the end of cunroll after ssa update and cfg cleanup
we have:
<bb 17> [local count: 18182121]:
# y_10 = PHI <y_20(D)(2), y_10(19)>
<bb 20> [local count: 2063999]:
# y_5 = PHI <y_10(18)>
PHIs for y and nothing else and finally phicprop2 optimizes all y references
to just y_20(D).
> > The following patch has 2 different fixes for this, one is to avoid looking
> > at SSA_NAMEs registered for update (in theory all we care are the old ssa
> > names, but we don't have an API to query just those), the other is to put
> > some upper bound on how many times we follow SSA_NAMEs (either single defs
> > or degenerate phis). Either of those changes is sufficient to fix this.
> Interestingly enough I was looking at a bug several weeks ago where the
> ability to query one of the sets (I can't remember if it was old or new)
> of names for rewriting would have been useful.
tree-cfg.c has a comment like that:
tree-cfg.c- /* Technically only new names matter. */
tree-cfg.c: if (name_registered_for_update_p (PHI_RESULT (phi)))
tree-cfg.c- return false;
Jakub
