[llvm-commits] Patch for simplifying regions
Tobias Grosser
grosser at fim.uni-passau.de
Tue Mar 1 05:49:37 PST 2011
On 02/28/2011 02:52 PM, Vu Le wrote:
> Hi Tobias,
> I did make some changes to use splitBlockPredecessors in splitting
> entry nodes.
Thanks for your fast reaction. The patch looks very nice. Though I have
some comments that we need to fix before committing it.
> I tested it with coreutils and MySQL together with region-extractor pass.
Nice.
> If you're still interested in region-extractor pass, I'll make a patch.
Yes. I am extremely interested.
> diff --git a/include/llvm/InitializePasses.h b/include/llvm/InitializePasses.h
> index 02dbfbd..0591845 100644
> --- a/include/llvm/InitializePasses.h
> +++ b/include/llvm/InitializePasses.h
> @@ -192,6 +192,7 @@ void initializeRegionInfoPass(PassRegistry&);
> void initializeRegionOnlyPrinterPass(PassRegistry&);
> void initializeRegionOnlyViewerPass(PassRegistry&);
> void initializeRegionPrinterPass(PassRegistry&);
> +void initializeRegionSimplifyPass(PassRegistry&);
> void initializeRegionViewerPass(PassRegistry&);
> void initializeRegisterCoalescerAnalysisGroup(PassRegistry&);
> void initializeRenderMachineFunctionPass(PassRegistry&);
> diff --git a/include/llvm/LinkAllPasses.h b/include/llvm/LinkAllPasses.h
> index 69e1bd9..ea1faec 100644
> --- a/include/llvm/LinkAllPasses.h
> +++ b/include/llvm/LinkAllPasses.h
> @@ -114,7 +114,8 @@ namespace {
> (void) llvm::createRegionInfoPass();
> (void) llvm::createRegionOnlyPrinterPass();
> (void) llvm::createRegionOnlyViewerPass();
> - (void) llvm::createRegionPrinterPass();
> + (void) llvm::createRegionPrinterPass();
This change does not seem to be needed. Please remove it.
> + (void) llvm::createRegionSimplifyPass();
> (void) llvm::createRegionViewerPass();
> (void) llvm::createSCCPPass();
> (void) llvm::createScalarReplAggregatesPass();
> diff --git a/include/llvm/Transforms/Scalar.h b/include/llvm/Transforms/Scalar.h
> index 6f2a38e..e3ca06a 100644
> --- a/include/llvm/Transforms/Scalar.h
> +++ b/include/llvm/Transforms/Scalar.h
> @@ -349,6 +349,12 @@ Pass *createCorrelatedValuePropagationPass();
> FunctionPass *createInstructionSimplifierPass();
> extern char&InstructionSimplifierID;
>
> +//===----------------------------------------------------------------------===//
> +//
> +// RegionSimplify - Simplify refined regions, if possible.
Are there cases when it is not possible? Would be interesting to mention
those.
> +Pass *createRegionSimplifyPass();
> +
> } // End llvm namespace
>
> #endif
> diff --git a/lib/Transforms/Scalar/CMakeLists.txt b/lib/Transforms/Scalar/CMakeLists.txt
> index 106fb8f..53fcf69 100644
> --- a/lib/Transforms/Scalar/CMakeLists.txt
> +++ b/lib/Transforms/Scalar/CMakeLists.txt
> @@ -23,6 +23,7 @@ add_llvm_library(LLVMScalarOpts
> MemCpyOptimizer.cpp
> Reassociate.cpp
> Reg2Mem.cpp
> + RegionSimplify.cpp
> SCCP.cpp
> Scalar.cpp
> ScalarReplAggregates.cpp
> diff --git a/lib/Transforms/Scalar/RegionSimplify.cpp b/lib/Transforms/Scalar/RegionSimplify.cpp
> new file mode 100644
> index 0000000..65e4d5c
> --- /dev/null
> +++ b/lib/Transforms/Scalar/RegionSimplify.cpp
> @@ -0,0 +1,185 @@
> +//===- SeSeRegionInfo.cpp -------------------------------------------------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file converts refined regions detected by the RegionInfo analysis
> +// into simple regions.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "llvm/Instructions.h"
> +#include "llvm/ADT/Statistic.h"
> +#include "llvm/Analysis/Dominators.h"
> +#include "llvm/Analysis/RegionPass.h"
> +#include "llvm/Analysis/RegionInfo.h"
> +#include "llvm/Transforms/Utils/BasicBlockUtils.h"
> +
> +#define DEBUG_TYPE "regionsimplify"
Can we call the pass "region-simplify"? Like "loop-simplify" (The name
was recently changed)
> +using namespace llvm;
> +
> +STATISTIC(NumEntries, "The # of created entry edges");
> +STATISTIC(NumExits, "The # of created exit edges");
> +
> +namespace {
> +class RegionSimplify: public RegionPass {
> + bool modified;
> + Region *CR;
> + void createSingleEntryEdge(Region *R);
> + void createSingleExitEdge(Region *R);
> +public:
> + static char ID;
> + explicit RegionSimplify() :
> + RegionPass(ID) {
> + initializeRegionSimplifyPass(*PassRegistry::getPassRegistry());
> + }
> +
> + virtual void print(raw_ostream&O, const Module *M) const;
> +
> + virtual bool runOnRegion(Region *R, RGPassManager&RGM);
> + virtual void getAnalysisUsage(AnalysisUsage&AU) const;
> +};
> +}
> +
> +INITIALIZE_PASS(RegionSimplify, "regionsimplify",
"regionsimplify" -> "region-simplify"
> + "Transform refined regions into simple regions", false, false)
> +
> +char RegionSimplify::ID = 0;
> +namespace llvm {
> +Pass *createRegionSimplifyPass() {
> + return new RegionSimplify();
> +}
> +}
> +
> +void RegionSimplify::print(raw_ostream&O, const Module *M) const {
> + if (!modified)
> + return;
> +
> + BasicBlock *enteringBlock = CR->getEnteringBlock();
> + BasicBlock *exitingBlock = CR->getExitingBlock();
> +
> + O<< "\nRegion: ["<< CR->getNameStr()<< "] Edges:\n";
> + if (enteringBlock)
> + O<< " Entry: ]"<< enteringBlock->getNameStr()<< " => "
> +<< enteringBlock->getNameStr()<< "]\n";
> + if (exitingBlock)
> + O<< " Exit: ["<< exitingBlock->getNameStr()<< " => "
> +<< exitingBlock->getNameStr()<< "[\n";
> +
> + O<< "\n";
> +}
> +
> +void RegionSimplify::getAnalysisUsage(AnalysisUsage&AU) const {
> + AU.addPreserved<DominatorTree> ();
> + AU.addPreserved<RegionInfo> ();
> + AU.addRequired<RegionInfo> ();
> +}
> +
> +// createSingleEntryEdge - Split the entry basic block of the given
> +// region after the last PHINode to form a single entry edge.
> +void RegionSimplify::createSingleEntryEdge(Region *R) {
> + BasicBlock *oldEntry = R->getEntry();
> + SmallVector<BasicBlock*, 4> Preds;
> + for (pred_iterator PI = pred_begin(oldEntry), PE = pred_end(oldEntry);
> + PI != PE; ++PI)
> + if (!R->contains(*PI))
> + Preds.push_back(*PI);
> +
> + if (Preds.size()==0)
> + return;
Can we change this to an assert?
assert(Preds.size() && "This region has already a single entry edge");
> +
> + BasicBlock *newEntry = SplitBlockPredecessors(oldEntry,
> + Preds.data(), Preds.size(),
> + ".single_entry", this);
> +
> + RegionInfo *RI =&getAnalysis<RegionInfo> ();
> + // We do not update entry node for children of this region.
> + // This make it easier to extract children regions because they do not share
> + // the entry node with their parents.
> + // all parent regions whose entry is oldEntry are updated with newEntry
> + Region *r = R->getParent();
> + while (r->getEntry() == oldEntry&& !r->isTopLevelRegion()) {
> + r->replaceEntry(newEntry);
> + r = r->getParent();
> + }
> +
> + // We do not update exit node for children of this region for the same reason
> + // of not updating entry node.
> + // All other regions whose exit is oldEntry are updated with new exit node
> + r = RI->getTopLevelRegion();
r = r->getParent() should be enough?
> + std::deque<Region *> RQ;
std::vector is probably simpler
> + RQ.push_back(r);
> + while (!RQ.empty()){
> + r = RQ.front();
> + for (Region::const_iterator RI = r->begin(), RE = r->end(); RI!=RE; ++RI)
> + RQ.push_back(*RI);
> + if (r->getExit() == oldEntry&& !R->contains(r))
> + r->replaceExit(newEntry);
> + RQ.pop_front();
> + }
I need to think about this part (Will send feedback later).
> +
> + modified |= true;
I do not believe this is part of creating the edges. 'modified' is part
of the status tracking of the pass and should probably be put into
runOnRegion().
> + ++NumEntries;
Dito. Please put in runOnRegion().
> +}
> +
> +// createSingleExitEdge - Split the exit basic of the given region
> +// to form a single exit edge.
> +void RegionSimplify::createSingleExitEdge(Region *R) {
> + BasicBlock *oldExit = R->getExit();
> +
> + SmallVector<BasicBlock*, 4> Preds;
> + for (pred_iterator PI = pred_begin(oldExit), PE = pred_end(oldExit);
> + PI != PE; ++PI)
> + if (R->contains(*PI))
> + Preds.push_back(*PI);
> +
> + BasicBlock *newExit = SplitBlockPredecessors(oldExit,
> + Preds.data(), Preds.size(),
> + ".single_exit", this);
> + RegionInfo *RI =&getAnalysis<RegionInfo> ();
> +
> + // We do not need to update entry nodes because this split happens inside
> + // this region and it affects only this region and all of its children.
> + // The new split node belongs to this region
> + RI->setRegionFor(newExit,R);
> +
> + // all children of this region whose exit is oldExit is changed to newExit
> + std::deque<Region *> RQ;
> + for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE; ++RI)
> + RQ.push_back(*RI);
> + while (!RQ.empty()){
> + R = RQ.front();
> + for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE; ++RI)
> + RQ.push_back(*RI);
> +
> + if (R->getExit() == oldExit)
> + R->replaceExit(newExit);
> + RQ.pop_front();
> + }
A vector is probably simpler. We also do not need to add child regions
for regions that do not have an exit matching oldExit. What about this code?
std::vector<Region *> RQ;
while (!RQ.empty()){
R = RQ.back();
RQ.pop_back();
if (R->getExit() != oldExit)
continue;
R->replaceExit(newExit);
for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE;
++RI)
RQ.push_back(*RI);
}
> +
> + modified |= true;
This function always modifies the CFG. So no need to put additional
logic here.
> + ++NumExits;
Dito.
> +}
> +
> +bool RegionSimplify::runOnRegion(Region *R, RGPassManager&RGM) {
> + modified = false;
> +
> + CR = R;
> + if (!R->isTopLevelRegion()) {
> +
> + if (!(R->getEnteringBlock())) {
> + createSingleEntryEdge(R);
> + }
if (!(R->getEnteringBlock())) {
createSingleEntryEdge(R);
++NumEntries;
modified = true.
}
> +
> + if (!(R->getExitingBlock())) {
> + modified |= createSingleExitEdge(R);
if (!(R->getExitingBlock())) {
createSingleExitEdge(R);
++NumExits;
modified = true;
}
> + }
> + }
> +
> + return modified;
> +}
> diff --git a/lib/Transforms/Scalar/Scalar.cpp b/lib/Transforms/Scalar/Scalar.cpp
> index bf9ca6d..5d18f22 100644
> --- a/lib/Transforms/Scalar/Scalar.cpp
> +++ b/lib/Transforms/Scalar/Scalar.cpp
> @@ -51,6 +51,7 @@ void llvm::initializeScalarOpts(PassRegistry&Registry) {
> initializeMemCpyOptPass(Registry);
> initializeReassociatePass(Registry);
> initializeRegToMemPass(Registry);
> + initializeRegionSimplifyPass(Registry);
> initializeSCCPPass(Registry);
> initializeIPSCCPPass(Registry);
> initializeSROA_DTPass(Registry);
> diff --git a/test/Transforms/RegionSimplify/dg.exp b/test/Transforms/RegionSimplify/dg.exp
> new file mode 100644
> index 0000000..f200589
> --- /dev/null
> +++ b/test/Transforms/RegionSimplify/dg.exp
> @@ -0,0 +1,3 @@
> +load_lib llvm.exp
> +
> +RunLLVMTests [lsort [glob -nocomplain $srcdir/$subdir/*.{ll,c,cpp}]]
> diff --git a/test/Transforms/RegionSimplify/multi_exits.ll b/test/Transforms/RegionSimplify/multi_exits.ll
> new file mode 100644
> index 0000000..024971c
> --- /dev/null
> +++ b/test/Transforms/RegionSimplify/multi_exits.ll
> @@ -0,0 +1,15 @@
> +; RUN: opt -regionsimplify
Did you test this test case. I believe you should use a line like:
; RUN: opt -regionsimplify %s
Furthermore, we should also check the generated output.
; RUN: opt -regionsimplify %s | FileCheck %s
> +define void @f() nounwind {
; CHECK: @f()
; CHECK: br label %"1"
...
Add after the CHECK lines the output you expect.
The same comments apply to the other test files. I would also be
interested to have some test files that include PHI-Nodes.
Can you send out an updated patch?
Thanks a lot
Tobi
More information about the llvm-commits
mailing list