[PATCH] D63338: PHINode: introduce setIncomingValueForBlock() function, and use it.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM, please wait for @fhahn's ok as well.



================
Comment at: llvm/include/llvm/IR/Instructions.h:2729
+  void setIncomingValueForBlock(const BasicBlock *BB, Value *V) {
+    assert(BB && "PHI node got a null basic block!");
+    for (unsigned Op = 0, NumOps = getNumOperands(); Op != NumOps; ++Op)
----------------
Whitney wrote:
> fhahn wrote:
> > Whitney wrote:
> > > fhahn wrote:
> > > > can this just be 
> > > > ```
> > > >     int Idx = getBasicBlockIndex(BB);
> > > >     assert(Idx >= 0 && "Invalid basic block argument!");
> > > >     return setIncomingValue(Idx, V);
> > > > ```
> > > > 
> > > > similar to getIncomingValueForBlock above?
> > > I thought of doing that, but sometimes a phi node may have multiple operands of the same incoming block (not sure why). I am using a similar code pattern as in replaceIncomingBlockWith().
> > Ah right. I think it is worth mentioning in the doc comment that it will replace all values coming from BB? 
> > 
> > For the places that use getbasicBlockIndex, this is not a NFC I think, as they would only replace the incoming value of the first occurrence.
> Removed [NFC] from the title. And updated the comment.
It's not forbidden and sometimes cannot even be simplified by fusing edges, e.g. a switch with multiple values branching to the same block. 

Unfortunately for the PHI it is indistinguishable which of the switch cases was taken. That means, all of the incoming values from the same block should be the same. This patch may actually fix some bugs where only one of the incoming values was changed.


================
Comment at: llvm/include/llvm/IR/Instructions.h:2677
   /// Replace every incoming basic block \p Old to basic block \p New.
-  void replaceIncomingBlockWith(BasicBlock *Old, BasicBlock *New) {
+  void replaceIncomingBlockWith(const BasicBlock *Old, BasicBlock *New) {
     assert(New && Old && "PHI node got a null basic block!");
----------------
[nit] unrelated


================
Comment at: llvm/include/llvm/IR/Instructions.h:2733
+        setIncomingValue(Op, V);
+  }
+
----------------
[suggestion] assert when no matching block was found?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63338/new/

https://reviews.llvm.org/D63338