[PATCH] D11683: Create a wrapper pass for MachineDominatorTree to remove pass dependence when it is only used conditionally.

[Cong Hou]

On Fri, Jul 31, 2015 at 5:08 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Jul-31, at 10:44, Cong Hou <congh at google.com> wrote:
>>
>> congh created this revision.
>> congh added reviewers: dexonsmith, dblaikie.
>> congh added subscribers: llvm-commits, davidxl.
>> Herald added subscribers: qcolombet, MatzeB.
>>
>> This patch adds a wrapper pass MachineDominatorTreeWrapperPass that contains MachineDominatorTree analysis, which could be done flexibly without declaring pass dependence. This is good to the compilation time: in some cases we only need to do this analysis conditionally (like in MachineBlockPlacement in this patch). This is quite similar to the wrapper pass for DominatorTree.
>>
>> Most changes are mechanical due to the name and usage changes.
>>
>> http://reviews.llvm.org/D11683
>>
>> Files:
>>  include/llvm/CodeGen/MachineDominators.h
>>  include/llvm/InitializePasses.h
>>  lib/CodeGen/CodeGen.cpp
>>  lib/CodeGen/EarlyIfConversion.cpp
>>  lib/CodeGen/InlineSpiller.cpp
>>  lib/CodeGen/LiveDebugVariables.cpp
>>  lib/CodeGen/LiveIntervalAnalysis.cpp
>>  lib/CodeGen/MachineBasicBlock.cpp
>>  lib/CodeGen/MachineBlockPlacement.cpp
>>  lib/CodeGen/MachineCSE.cpp
>>  lib/CodeGen/MachineCombiner.cpp
>>  lib/CodeGen/MachineDominanceFrontier.cpp
>>  lib/CodeGen/MachineDominators.cpp
>>  lib/CodeGen/MachineLICM.cpp
>>  lib/CodeGen/MachineLoopInfo.cpp
>>  lib/CodeGen/MachineRegionInfo.cpp
>>  lib/CodeGen/MachineScheduler.cpp
>>  lib/CodeGen/MachineSink.cpp
>>  lib/CodeGen/PHIElimination.cpp
>>  lib/CodeGen/PeepholeOptimizer.cpp
>>  lib/CodeGen/PostRASchedulerList.cpp
>>  lib/CodeGen/PrologEpilogInserter.cpp
>>  lib/CodeGen/RegAllocBasic.cpp
>>  lib/CodeGen/RegAllocGreedy.cpp
>>  lib/CodeGen/RegAllocPBQP.cpp
>>  lib/CodeGen/ShrinkWrap.cpp
>>  lib/CodeGen/StackColoring.cpp
>>  lib/CodeGen/UnreachableBlockElim.cpp
>>  lib/Target/AArch64/AArch64CleanupLocalDynamicTLSPass.cpp
>>  lib/Target/AArch64/AArch64CollectLOH.cpp
>>  lib/Target/AArch64/AArch64ConditionOptimizer.cpp
>>  lib/Target/AArch64/AArch64ConditionalCompares.cpp
>>  lib/Target/AMDGPU/AMDILCFGStructurizer.cpp
>>  lib/Target/AMDGPU/R600OptimizeVectorRegisters.cpp
>>  lib/Target/AMDGPU/R600Packetizer.cpp
>>  lib/Target/AMDGPU/SIFoldOperands.cpp
>>  lib/Target/AMDGPU/SILowerI1Copies.cpp
>>  lib/Target/Hexagon/HexagonFrameLowering.cpp
>>  lib/Target/Hexagon/HexagonGenInsert.cpp
>>  lib/Target/Hexagon/HexagonGenPredicate.cpp
>>  lib/Target/Hexagon/HexagonHardwareLoops.cpp
>>  lib/Target/Hexagon/HexagonVLIWPacketizer.cpp
>>  lib/Target/Mips/MipsOptimizePICCall.cpp
>>  lib/Target/PowerPC/PPCCTRLoops.cpp
>>  lib/Target/SystemZ/SystemZLDCleanup.cpp
>>  lib/Target/X86/X86InstrInfo.cpp
>>
>> <D11683.31132.patch>
>
> I have some comments on the MachineBlockPlacement changes (I'm not
> comfortable signing off even after you address my comments, though,
> so someone else will have to chime in).
>
>> Index: lib/CodeGen/MachineBlockPlacement.cpp
>> ===================================================================
>> --- lib/CodeGen/MachineBlockPlacement.cpp
>> +++ lib/CodeGen/MachineBlockPlacement.cpp
>> @@ -202,7 +202,7 @@
>>    const TargetLoweringBase *TLI;
>>
>>    /// \brief A handle to the post dominator tree.
>> -  MachineDominatorTree *MDT;
>> +  std::unique_ptr<MachineDominatorTree> MDT;
>
> I don't see you releasing this memory anywhere.  Should you?
>
>>
>>    /// \brief A set of blocks that are unavoidably execute, i.e. they dominate
>>    /// all terminators of the MachineFunction.
>> @@ -262,7 +262,6 @@
>>    void getAnalysisUsage(AnalysisUsage &AU) const override {
>>      AU.addRequired<MachineBranchProbabilityInfo>();
>>      AU.addRequired<MachineBlockFrequencyInfo>();
>> -    AU.addRequired<MachineDominatorTree>();
>
> If the MDT is already available, shouldn't we use it?  Should this be
> changed to `addOptional<>`?

I didn't realize the results from a pass can be reused in later passes
if they are preserved. Actually, I found this is the case for
MachineDominatorTree in MachineBlockPlacement: in the previous pass
PostRAScheduler, MachineDominatorTree is created and also preserved.
So the optimization done in this patch (at least for
MachineBlockPlacement) is not necessary.

As the pass MachineBlockPlacement doesn't change the results of the
required passes here (like MBP, MBF, and MachineDominatorTree), I
think it would be better to make them preserved (probably this is also
unnecessary as MachineBlockPlacement is almost the last pass).

>
>>      AU.addRequired<MachineLoopInfo>();
>>      MachineFunctionPass::getAnalysisUsage(AU);
>>    }
>> @@ -275,7 +274,6 @@
>>                        "Branch Probability Basic Block Placement", false, false)
>>  INITIALIZE_PASS_DEPENDENCY(MachineBranchProbabilityInfo)
>>  INITIALIZE_PASS_DEPENDENCY(MachineBlockFrequencyInfo)
>> -INITIALIZE_PASS_DEPENDENCY(MachineDominatorTree)
>>  INITIALIZE_PASS_DEPENDENCY(MachineLoopInfo)
>>  INITIALIZE_PASS_END(MachineBlockPlacement, "block-placement",
>>                      "Branch Probability Basic Block Placement", false, false)
>> @@ -1138,7 +1136,8 @@
>>    MLI = &getAnalysis<MachineLoopInfo>();
>>    TII = F.getSubtarget().getInstrInfo();
>>    TLI = F.getSubtarget().getTargetLowering();
>> -  MDT = &getAnalysis<MachineDominatorTree>();
>> +  if (OutlineOptionalBranches)
>> +    MDT.reset(new MachineDominatorTree(F));
>>    assert(BlockToChain.empty());
>>
>>    buildCFGChains(F);
>>
>