[PATCH] D28934: Write a new SSAUpdater

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D28934#768477, @davide wrote:

> @dberlin I just came back to this while I was looking at NewGVN PRE.
>  I decided to give it a try to see how the API looked like and I personally didn't find any problems with it (I might just used it wrong, FWIW).
>
> https://reviews.llvm.org/P7997
>
> In particular, the old API `getValueInTheMiddleOfTheBlock` has been source of confusion (at least to me).
>
> With your patch and my patch on top of it, I saw still one test failing:
>
>   --- /home/davide/old.ll 2017-05-30 20:56:21.058869000 -0700
>   +++ /home/davide/new.ll 2017-05-30 20:56:30.450514797 -0700
>   @@ -316,7 +316,7 @@
>      br label %block4
>  
>    block4:                                           ; preds = %block3, %block2
>   -  %D = phi i32 [ 87, %block2 ], [ 97, %block3 ]
>   +  %D = phi i32 [ 97, %block3 ], [ 87, %block2 ]
>      %A = phi i32 [ -1, %block2 ], [ 42, %block3 ]
>      br i1 %cmpxy, label %block5, label %exit
>  
>   @@ -350,7 +350,7 @@
>      br label %loop
>  
>    loop:                                             ; preds = %loop, %entry
>   -  %Y2 = phi i8 [ %Y, %entry ], [ 0, %loop ]
>   +  %Y2 = phi i8 [ 0, %loop ], [ %Y, %entry ]
>      %i = phi i32 [ 4, %entry ], [ 192, %loop ]
>      %X2 = getelementptr i8, i8* %p, i32 %i
>      %cond = call i1 @cond2()
>   @@ -372,7 +372,7 @@
>      br label %loop
>  
>    loop:                                             ; preds = %cont, %entry
>   -  %Y2 = phi i8 [ %Y, %entry ], [ %Y2.pre, %cont ]
>   +  %Y2 = phi i8 [ %Y2.pre, %cont ], [ %Y, %entry ]
>      %i = phi i32 [ 4, %entry ], [ 3, %cont ]
>      %X2 = getelementptr i8, i8* %p, i32 %i
>      %cond = call i1 @cond2()
>
>
> I guess this is because we don't have a consistent ordering of PHI args. If that's the only problem, I might consider looking at it next (and then keep converting the remaining users).


Yes, it should hit the phi arguments in pred order, roughly, but it won't be perfect.
We should just sort them.

Note that we also will not go looking for existing phis to reuse.
IMHO, doing so (as current SSAUpdater does), is not a good plan.
Either let CSE/instsimplify/any of the million things that will fix this, clean it up
or we need to extend the lifetime of ssa updater objects and hash the arguments of phi nodes in blocks we see.
Right now, what SSAUpdater does is badly N^2.

Anyway, if you want to work on this patch, feel free to submit diffs and takeover this revision.  I probably won't get back to it for a few months at least.


https://reviews.llvm.org/D28934