[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:
> > > 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.
> 
How about splitting the current function into two, one for each case + one helper that detects the case and uses the appropriate version?
I have actually encountered this behavior several times with switch instructions (IR-level range splitting, etc) so I don't want to "keep it for myself".
Regarding the name: what name(s) would you propose? I have originally called this function `fixPHIs`, but later decided to rename it to `updatePHIs`. Maybe you have better suggestions.


https://reviews.llvm.org/D33787