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

[Bill Wendling]

Done here r91101. Thanks, Dan!

-bw

On Dec 10, 2009, at 7:01 PM, Dan Gohman wrote:

> 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 use that 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits