[PATCH] D33787: [IR] BasicBlock updatePHIs function for updating predecessors

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/IR/BasicBlock.cpp:466
+      if (oldPredecessor) {
+        // Cannot just change the incoming when IncomingOld is still valid,
+        // need to insert another value with the same incoming value.
----------------
Marandil wrote:
> dberlin wrote:
> > Marandil wrote:
> > > dberlin wrote:
> > > > Can you explain why?
> > > > This seems like  a bug?
> > > Consider this:
> > > 
> > >   switch (a) {
> > >     default: BB0;
> > >     case 0: BB0;
> > >     case 1: BB1;
> > >     case 2: BB2;
> > >     case 19: BB0;
> > >     case 20: BB1;
> > >     case 21: BB2;
> > >   }
> > > 
> > > changed into:
> > > 
> > >   if (0 <= a <=2) {
> > >     switch (a) {
> > >       default: unreachable;
> > >       case 0: BB0;
> > >       case 1: BB1;
> > >       case 2: BB2;
> > >     }
> > >   } else {
> > >     switch (a-19) {
> > >       default: BB0;
> > >       case 0: BB0;
> > >       case 1: BB1;
> > >       case 2: BB2;
> > >     }
> > >   }
> > > 
> > > For BB0 a single predecessor changed into 2 different possible predecessors, that should share the same incoming value.
> > That just makes me believe the API specification for this function is confusing.
> > Either your goal is to update all phis from one block to another, or it isn't.
> > In the above, you aren't.
> > 
> > 
> > 
> The specification says:
> 
>   /// \brief Replace (or add) occurrences of IncomingOld in PHIs with IncomingNew,
>   /// depending on whether or not IncomingOld is still a valid predecessor.
> 
> Consider a shorter example:
> 
>   BB0:
>     br i1 %0, label %BB2, label %BB2
>   BB1:
>     br label %BB2
>   BB2:
>     %1 = phi i32 [0, %BB0], [1, %BB1]
>     ret i32 %1
> 
> Now, I modify the second edge of the `br` in BB0 (oblivious on the dest of the first one)
> 
>   BB0:
>     br i1 %0, label %BB2, label %BB3
>   BB1:
>     br label %BB2
>   BB2:
>     %1 = phi i32 [0, %BB0], [1, %BB1]
>     ret i32 %1
>   BB3:
>     br label %BB0
> 
> Obviously, the PHI node in BB2 is now invalid, as BB3->BB2 is another valid edge; so we need to update it:
> 
>   BB0:
>     br i1 %0, label %BB2, label %BB3
>   BB1:
>     br label %BB2
>   BB2:
>     %1 = phi i32 [0, %BB0], [1, %BB1], [0, %BB3]
>     ret i32 %1
>   BB3:
>     br label %BB0
> 
> The function is supposed to detect whether or not a new value in PHI should be added and it is inclined in the description.
I understand you have documented what it does.
I'm saying 
A. "what it does" seems very specific to your use case
B. What it does in a specific case is not easy to understand just from looking at the call.
C.  It is not what i would expect from a function named "updatephis" ,  I would expect such a thing to do  *either* addition *or* replacement, not "add/replac depending on a bunch of variables and factors"
etc

IE This function seems incredibly specific to your case of updating.

I can say for certain in all the phi updating i've encountered, it would not be usable or useful.
A one to one replacement function would be useful (IE replace old with new)
An "add new edge to every phi" would be useful.
A function that tries to combine them and do something depending on other state that isn't passed - not so much.

But i'm going to leave it to others to see if they think this functionality is useful.
I can only request if they do, you don't call it "updatephis" because that is a very generic name for something that is quite specific.



https://reviews.llvm.org/D33787