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

[Marcin Słowik via Phabricator via llvm-commits]

Marandil 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.
----------------
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.


https://reviews.llvm.org/D33787