[llvm-commits] CVS: llvm/lib/CodeGen/BranchFolding.cpp

[Duncan Sands]

Hi Dale,

> > it struck me that this patch seems to assume that if a successor
> > is a landing pad then it is necessarily a landing pad for the
> > current MBB.
> 
> I wasn't intending to assume that; how so?
> 
> > But couldn't it be the landing pad for some other
> > MBB, and simply a normal successor for the current one?
> 
> It could; there is no way to tell from the CFG information.

consider the code:

  MachineBasicBlock::succ_iterator SI = MBB.succ_begin();
  bool foundPad = false;
  while (SI != MBB.succ_end()) {
    if (*SI == DestA && DestA == DestB) {
      DestA = DestB = 0;
      if ((*SI)->isLandingPad())
        foundPad = true;
      ++SI;
    } else if (*SI == DestA) {
      DestA = 0;
      if ((*SI)->isLandingPad())
        foundPad = true;
      ++SI;
    } else if (*SI == DestB) {
      DestB = 0;
      if ((*SI)->isLandingPad())
        foundPad = true;
      ++SI;
    } else if ((*SI)->isLandingPad() && !foundPad) {
      foundPad = true;
      ++SI;
    } else {
      // Otherwise, this is a superfluous edge, remove it.
      MBB.removeSuccessor(SI);
      MadeChange = true;
    }
  }

Here is the worrying bit:

    } else if ((*SI)->isLandingPad() && !foundPad) {
      foundPad = true;
      ++SI;
    } else {

Suppose a successor S1 has isLandingPad true because it is
a landing pad for some other MBB, and some later successor (S2)
has isLandingPad true because it is the landing pad for this
MBB.  What will happen?  When we get to S1 foundPad is set to
true and S1 is not deleted.  Later we get to S2 and, because
foundPad is true, fall through to
    } else {
      // Otherwise, this is a superfluous edge, remove it.
      MBB.removeSuccessor(SI);
      MadeChange = true;
    }
and wrongly delete S2.

Ciao,

Duncan.