On Wed, Dec 28, 2016 at 1:18 PM, Friedman, Eli <efriedma at codeaurora.org>
wrote:
> On 12/28/2016 1:03 PM, Daniel Berlin via llvm-commits wrote:
>
>
>
> On Wed, Dec 28, 2016 at 7:04 AM, Davide Italiano via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> davide accepted this revision.
>> davide added a comment.
>> This revision is now accepted and ready to land.
>>
>> Sorry for the slow response, I'm out('ish) of the office these days. I
>> took a close look at your patch.
>>
>
> No worries.
>
>
>> I happen to be lucky enough to hit a case in the wild where this already
>> matters. The number of iteration goes down from hundreds to ~10, which
>> makes compile time/me happier.
>>
>
> yay.
>
> The current code, excepting super-weird cases, should operate in O(d+3)
> iterations, where d is the loop connectedness of the SSA graph (not the
> CFG), which is the number of backedges in any path. This will change when
> we move to equality propagation, but for now, ...
> We could calculate this number and see if we are screwing up :)
>
> For most programs, the loop connectedness of the SSA graph is the same or
> less than the CFG.
>
> However, IIRC, we allow dependent phis in the same block (this is not
> strictly SSA, since all phi nodes are supposed to be evaluated
> simultaneously).
>
>
> I'm not sure what you're trying to say here? PHI nodes for a given basic
> block are evaluated simultaneously. From LangRef: "For the purposes of
> the SSA form, the use of each incoming value is deemed to occur on the edge
> from the corresponding predecessor block to the current block (but after
> any definition of an ‘invoke‘ instruction’s return value on the same
> edge)."
>
>
I'm saying we've had mailing list arguments about this, about whether there
is any ordering among phi nodes in a given block. The part you quote from
the langref does not actually definitively answer that (again, there is no
argument in theory. In the abstract, the answer is "there is no ordering,
it's undefined to have phis depend in the same block depend on each other")
Given
b = phi(d, e)
a = phi(b, c)
Saying "is deemed to occur on the edge of the corresponding predecessor
block" does not help.
If they are evaluated in the order we see them, it still comes out valid,
because one edge will be:
b = e
a = c
and the other
b = d
a = b
b still dominates all uses.
However, if they are evaluated simultaneously, then b does dominate a, by
the definition of dominance. It's not just a swap copy problem, it's a
"this is not well defined code" problem.
Last i remember,
1. We never bothered to write down the answer into the langref.
2. The verifier is fine with bad phis (assuming we do *not* allow the
above).
The following passes verification:
define void @widget() {
entry:
br label %bb2
bb2: ; preds = %bb4, %entry
%aa = phi i64 [ 5, %entry ], [ %t5, %bb4]
%t1 = phi i64 [ 0, %entry ], [ %aa, %bb4 ]
br label %bb4
bb4: ; preds = %bb3, %bb2
%t5 = add i64 %t1, 1
br label %bb2
}
So does:
define void @widget() {
entry:
br label %bb2
bb2: ; preds = %bb4, %entry
%t1 = phi i64 [ 0, %entry ], [ %aa, %bb4 ]
%aa = phi i64 [ 5, %entry ], [ %t5, %bb4]
br label %bb4
bb4: ; preds = %bb3, %bb2
%t5 = add i64 %t1, 1
br label %bb2
}
We've had this discussion before (here was 7 years ago, there is plenty
more :P):
http://lists.llvm.org/pipermail/llvm-dev/2009-January/019779.html
Owen is, AFAIK, still correct about "LLVM does not implement this
simultaneous evaluation for practical
reasons. "
Instead, our phi nodes are effectively executable, instead of just abstract.
Popping back up, regardless of resolution, this causes the issue i
mentioned above - it may require more iterations to resolve because of the
second case passing verification. If we really want phi nodes to be
executable , and want it to take the minimum number of iterations to
converge NewGVN, we need to process aa before t1.
Otherwise, we will process t1, get some value, *then* process aa, and
immediately mark t1 as needing to be reprocessed since it is a use of aa.
We effectively waste an iteration because all of t1's uses have are going
to have the wrong value.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161228/856a6059/attachment.html>