[llvm-commits] [llvm] r91092 - in /llvm/trunk: include/llvm/CodeGen/MachineBasicBlock.h lib/CodeGen/MachineBasicBlock.cpp

[Dan Gohman]

Hi Bill,

See below for a few comments.

On Dec 10, 2009, at 5:49 PM, Bill Wendling wrote:

> Author: void
> Date: Thu Dec 10 19:49:14 2009
> New Revision: 91092
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=91092&view=rev
> Log:
> A machine basic block may end in an unconditional branch, however it may have
> more than one successor. Normally, these extra successors are dead. However,
> some of them may branch to exception handling landing pads. If we remove those
> successors, then the landing pads could go away if all predecessors to it are
> removed. Before, it was checking if the direct successor was the landing
> pad. But it could be the result of jumping through multiple basic blocks to get
> to it. If we were to only check for the existence of an EH_LABEL in the basic
> block and not remove successors if it's in there, then it could stop actually
> dead basic blocks from being removed.
> 
> Modified:
>    llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h
>    llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp
> 
> Modified: llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h?rev=91092&r1=91091&r2=91092&view=diff
> 
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h Thu Dec 10 19:49:14 2009
> @@ -327,6 +327,11 @@
>   /// 'Old', change the code and CFG so that it branches to 'New' instead.
>   void ReplaceUsesOfBlockWith(MachineBasicBlock *Old, MachineBasicBlock *New);
> 
> +  /// BranchesToLandingPad - The basic block branches only to a landing pad or
> +  /// to another basic block which branches only to a landing pad. No other
> +  /// instructions are present other than the unconditional branch.
> +  bool BranchesToLandingPad(const MachineBasicBlock *MBB) const;
> +
>   /// CorrectExtraCFGEdges - Various pieces of code can cause excess edges in
>   /// the CFG to be inserted.  If we have proven that MBB can only branch to
>   /// DestA and DestB, remove any other MBB successors from the CFG. DestA and
> 
> Modified: llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp?rev=91092&r1=91091&r2=91092&view=diff
> 
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp Thu Dec 10 19:49:14 2009
> @@ -13,15 +13,16 @@
> 
> #include "llvm/CodeGen/MachineBasicBlock.h"
> #include "llvm/BasicBlock.h"
> +#include "llvm/ADT/SmallSet.h"
> +#include "llvm/Assembly/Writer.h"
> #include "llvm/CodeGen/MachineFunction.h"
> -#include "llvm/Target/TargetRegisterInfo.h"
> #include "llvm/Target/TargetData.h"
> #include "llvm/Target/TargetInstrDesc.h"
> #include "llvm/Target/TargetInstrInfo.h"
> #include "llvm/Target/TargetMachine.h"
> +#include "llvm/Target/TargetRegisterInfo.h"
> #include "llvm/Support/LeakDetector.h"
> #include "llvm/Support/raw_ostream.h"
> -#include "llvm/Assembly/Writer.h"
> #include <algorithm>
> using namespace llvm;
> 
> @@ -448,10 +449,35 @@
>   addSuccessor(New);
> }
> 
> +/// BranchesToLandingPad - The basic block branches only to a landing pad or to
> +/// another basic block which branches only to a landing pad. No other
> +/// instructions are present other than the unconditional branch.

Looking at the code, this function returns true if the given MBB itself
is a landing pad, not just if it branches to one; please mention this
in the comment.

> +bool
> +MachineBasicBlock::BranchesToLandingPad(const MachineBasicBlock *MBB) const {
> +  SmallSet<const MachineBasicBlock*, 32> Visited;
> +  const MachineBasicBlock *CurMBB = MBB;
> +
> +  while (!Visited.count(CurMBB) && !CurMBB->isLandingPad()) {
> +    if (CurMBB->size() != 1 || CurMBB->succ_empty() || CurMBB->succ_size() != 1)

Since you're checking for succ_size() != 1, the succ_empty() check is
redundant.

> +      break;
> +
> +    const TargetInstrInfo *TII =
> +      CurMBB->getParent()->getTarget().getInstrInfo();
> +    if (!TII->isUnpredicatedTerminator(CurMBB->begin()))
> +      break;

This check seems over-specific.

> +
> +    Visited.insert(CurMBB);

insert returns false if the element was already present, so if you want
you can reorganize things to avoid needing to do another lookup (the
count call) each iteration.

> +    CurMBB = *CurMBB->succ_begin();
> +  }
> +
> +  return CurMBB->isLandingPad();
> +}
> +
> /// CorrectExtraCFGEdges - Various pieces of code can cause excess edges in the
> /// CFG to be inserted.  If we have proven that MBB can only branch to DestA and
> /// DestB, remove any other MBB successors from the CFG.  DestA and DestB can
> /// be null.
> +/// 
> /// Besides DestA and DestB, retain other edges leading to LandingPads
> /// (currently there can be only one; we don't check or require that here).
> /// Note it is possible that DestA and/or DestB are LandingPads.
> @@ -481,16 +507,17 @@
>   }
> 
>   MachineBasicBlock::succ_iterator SI = succ_begin();
> -  MachineBasicBlock *OrigDestA = DestA, *OrigDestB = DestB;
> +  const MachineBasicBlock *OrigDestA = DestA, *OrigDestB = DestB;
>   while (SI != succ_end()) {
> -    if (*SI == DestA) {
> +    const MachineBasicBlock *MBB = *SI;
> +    if (MBB == DestA) {
>       DestA = 0;
>       ++SI;
> -    } else if (*SI == DestB) {
> +    } else if (MBB == DestB) {
>       DestB = 0;
>       ++SI;
> -    } else if ((*SI)->isLandingPad() && 
> -               *SI!=OrigDestA && *SI!=OrigDestB) {
> +    } else if (BranchesToLandingPad(MBB) &&
> +               MBB != OrigDestA && MBB != OrigDestB) {

The != tests here are relatively cheap compared to the
BranchesToLandingPad call, so it would be good to check
them first.

Dan

>       ++SI;
>     } else {
>       // Otherwise, this is a superfluous edge, remove it.
> @@ -498,12 +525,14 @@
>       MadeChange = true;
>     }
>   }
> +
>   if (!AddedFallThrough) {
>     assert(DestA == 0 && DestB == 0 &&
>            "MachineCFG is missing edges!");
>   } else if (isCond) {
>     assert(DestA == 0 && "MachineCFG is missing edges!");
>   }
> +
>   return MadeChange;
> }
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits