Move insert point after PHI nodes.

[zoecarver]

If replacing a PHI node with another instruction, make sure the insertion point is after the last PHI node in that block. This prevents the assertion described in #779.

[zoecarver]

I was having a bit of a hard time coming up with good tests. If I could tell souper to just run once, I think I could create a better test (that checks we replace a PHI instruction with an add, for example).

Other than that, I can't come up with an input program where the PHI node is replaced with a non-phi node, and the whole thing isn't optimized away at some point later.

[regehr]

ahhh... thanks for looking into this! we just haven't had time. I'll look at the code tomorrow, but I think for now it's sufficient testing to include the original bug trigger as a regression test. we'll get plenty more testing using our existing fuzzers.

[regehr]

@zhengyangl do you have time to review this request?

[zhengyang92]

Hi @zoecarver ,

Thanks for the patch! One concern: as stated in [1], after calling IRBuilder.SetInsertPoint(Instruction *I), the new instructions will be inserted before instruction I. In this case, InsertPoint should be the instruction just next to the last phi in that basic block, right? However, this code would make InsertPoint the last PHI and Instruction could still possibly be inserted between PHI nodes.

[1] https://llvm.org/doxygen/classllvm_1_1IRBuilderBase.html#ae72654261eee259da46249dc919b029c

[zoecarver]

You're right. Fixed.

[zhengyang92]

Since IR builder is inserting before the ReplacedInst, it should work even when ReplacedInst is a terminator, right? Therefore, checking the terminator is not necessary here.

And Souper won't create Phis, the check I->K != Inst::Kind::Phi is not necessary here.

I think

if (ReplacedInst) {
  Instruction *InsertPoint = ReplacedInst;
  while (isa<PHINode>(InsertPoint) {
    InsertPoint = InsertPoint->getNextNode();
  }
  Builder.SetInsertPoint(InsertPoint);
}

should just do the work.

[zoecarver]

Good point. I'll update it with your suggestion. I think it would be good to keep the isa<PHINode>(ReplacedInst->getNextNode()) check so that we don't unnecessarily update the InsertPoint, though.

[zhengyang92]

Hi @zoecarver, one final note: if some replacement has Phi for ReplacedInst and a non-phi for the replacedInst->getNextNode, then we'll never reach the true branch, and by the definition of setInsertPoint() the new instruction would be inserted before the ReplaceInst (a phi node), which will cause trouble since we're trying to insert a non-phi instruction in the middle of the phi chunk.

if (ReplacedInst && isa<PHINode>(Replacement)) would be bug-free.

[zoecarver]

Hi @zoecarver, one final note: if some replacement has Phi for ReplacedInst and a non-phi for the replacedInst->getNextNode, then we'll never reach the true branch, and by the definition of setInsertPoint() the new instruction would be inserted before the ReplaceInst (a phi node), which will cause trouble since we're trying to insert a non-phi instruction in the middle of the phi chunk.

Sorry for the delayed response. My reasoning is that ReplacedInst will always be removed after the new instruction is added. In this case, we don't want to change the InsertPoint unnecessarily because even though for a short period of time there will be a non-phi node before a phi node, the replaced phi node will be removed before the verifier ever runs.

[regehr]

hey @zhengyangl are you OK with this patch now? it looks fine to me

[zhengyang92]

LGTM

[regehr]

thanks for the bugfix!

[zoecarver]

thanks for the bugfix!

Of course. Thanks (both of you) for reviewing/merging!