[llvm-commits] Patch for simplifying regions
Tobias Grosser
grosser at fim.uni-passau.de
Wed Mar 2 14:22:06 PST 2011
On 03/02/2011 03:08 PM, Vu Le wrote:
> Add more test cases.
Hi,
this looks already very good. Though I still have a bunch of comments (I
did not spend enough time reviewing it before. Sorry)
Comments inline
> Vu
> 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..6764543 100644
> --- a/include/llvm/LinkAllPasses.h
> +++ b/include/llvm/LinkAllPasses.h
> @@ -115,6 +115,7 @@ namespace {
> (void) llvm::createRegionOnlyPrinterPass();
> (void) llvm::createRegionOnlyViewerPass();
> (void) llvm::createRegionPrinterPass();
> + (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..330fa15 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.
> +//
> +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..9c78269
> --- /dev/null
> +++ b/lib/Transforms/Scalar/RegionSimplify.cpp
> @@ -0,0 +1,191 @@
> +//===- RegionSimplify.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 "region-simplify"
> +
> +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, "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";
I believe this should be something like:
O<< " Entry: ]"<< enteringBlock->getNameStr()<< " => "
<< CR->getEntry()->getNameStr()<< "]\n";
No need to update the condition as all regions have a entry node.
> + if (exitingBlock)
> + O<< " Exit: ["<< exitingBlock->getNameStr()<< " => "
> +<< exitingBlock->getNameStr()<< "[\n";
if (exitingBlock && CR->getExit())
O<< " Exit: ["<< exitingBlock->getNameStr()<< " => "
<< CR->getExit() << "[\n";
Here we need to update the condition as the toplevel region does not
have an exit node.
Unfortunately I could not test this as '-analyze' leads to a crash:
opt -region-simplify phi_entry.ll -analyze
Printing analysis 'Transform refined regions into simple regions' for
region: 'l4 => exit' in function 'f':
Stack dump:
0. Program arguments: opt -load
/home/grosser/Projekte/polly/build_clang/lib/LLVMPolly.so
-region-simplify phi_entry.ll -analyze
1. Running pass 'Function Pass Manager' on module 'phi_entry.ll'.
2. Running pass 'Region Pass Manager' on function '@f'
3. Running pass 'RegionPass Printer: Transform refined regions into
simple regions' on basic block '%l4'
Segmentation fault
Similar crashes for the other test cases. Can you reproduce this?
We should add a call to -analysis to the test suite, after this is fixed.
> +
> + O<< "\n";
> +}
> +
> +void RegionSimplify::getAnalysisUsage(AnalysisUsage&AU) const {
> + AU.addPreserved<DominatorTree> ();
> + AU.addPreserved<RegionInfo> ();
> + AU.addRequired<RegionInfo> ();
Please remove the space before the '()'.
> +}
> +
> +// 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) {
Sorry for bugging you again. I changed my mind. Splitting
NewExit->OldExit is incorrect. As we need to update Regions that are not
descendants of R. I would like to follow the ideas of the LoopPasses and
disallow to change anything outside of the Loop/Region
currently worked on.
Can you just add the code we had in the original patch? I also added
some more comments:
+// createSingleEntryEdge - Split the entry BasicBlock of the given
+// region after the last PHINode to form a single entry edge.
+// This is similar to CodeExtractor::severSplitPHINodes
+BasicBlock *RegionSimplify::createSingleEntryEdge(Region *R) {
+ Function *f = R->getEntry()->getParent();
+ if (&f->getEntryBlock() == R->getEntry())
+ return NULL; // Entry node is the function's entry blocks
This can now be removed
+ BasicBlock *oldEntry = R->getEntry();
+ PHINode *PN = dyn_cast<PHINode> (oldEntry->begin());
+ if (!PN)
+ return NULL; // No PHI nodes.
I do not see a reason why we should not create a single entry edge, just
because there are no PHI nodes.
+
+ BasicBlock::iterator AfterPHIs = oldEntry->getFirstNonPHI();
+ BasicBlock *newEntry = oldEntry->splitBasicBlock(AfterPHIs,
+ oldEntry->getName() + ".simregentry");
region_exit
+ // Okay, update dominator sets.
+ if (DominatorTree *DT = getAnalysisIfAvailable<DominatorTree>()) {
+ succ_iterator secondSucc = succ_begin(newEntry) + 1;
+ if (secondSucc == succ_end(newEntry)) //newEntry has 1 successor
+ DT->splitBlock(newEntry);
+ else { // newEntry has more than 1 successor, update DT manually
+ // oldEntry dominates newEntry.
So this should be moved up here. We always want to newEntry to the
dominator tree
DomTreeNode *NewNode = DT->addNewBlock(newEntry, OldEntry);
+ // newEntry node dominates all other nodes dominated by oldEntry.
+ DomTreeNode *OldNode = DT->getNode(oldEntry);
+ if (OldNode) { // don't bother if oldEntry doesn't dominates any node
No need for this extra check. In case oldEntry does not dominate
anything the loop will just not be executed. Adding more code, if a
relevant performance improvement is not shown, is something I would like
to avoid.
+ std::vector<DomTreeNode *> Children;
+ for (DomTreeNode::iterator I = OldNode->begin(), E =
OldNode->end(); I
+ != E; ++I)
+ Children.push_back(*I);
+
+ DomTreeNode *NewNode = DT->addNewBlock(newEntry, oldEntry);
+ for (std::vector<DomTreeNode *>::iterator I = Children.begin(), E =
+ Children.end(); I != E; ++I)
+ DT->changeImmediateDominator(*I, NewNode);
+ }
+ }
+ }
+
+ // Loop over all of the predecessors of the old entry that are in the
region,
+ // changing them to branch to the new entry instead of the old one
A little shorter would be:
// Change all predecessors of the old entry, that are part of the
// region, to branch to the new entry instead of the old one.
+ for (pred_iterator PI = pred_begin(oldEntry), PE = pred_end(oldEntry); PI
+ != PE; ++PI) {
+ if (R->contains(*PI)) {
+ TerminatorInst *TI = (*PI)->getTerminator();
+ TI->replaceUsesOfWith(oldEntry, newEntry);
+ }
+ }
+
+ // Okay, everthing within the region is now branching to the right
block, we
+ // just have to update the PHI nodes now, inserting PHI nodes into NewBB.
A shorter comment:
// Insert required PHI nodes into NewBB.
+ for (BasicBlock::iterator PI = oldEntry->begin(); isa<PHINode> (PI);
++PI) {
It is probably faster to do:
for (BasicBlock::iterator PI = oldEntry->begin(),
PE = oldEntry->getFirstNonPHI(); PI != PE; ++PI)
+ PHINode *PN = cast<PHINode> (PI);
+ // Create a new PHI node in the new region, which has an incoming value
+ // from oldEntry of PN.
+ PHINode *NewPN = PHINode::Create(PN->getType(), PN->getName() + ".ph",
+ newEntry->begin());
+
+ NewPN->addIncoming(PN, oldEntry);
+
+ // Loop over all of the incoming value in PN, moving them to NewPN
if they
+ // are from the region.
Removing the 'Loop over'. No need to explain the for loop:
// Move the incoming values of PN to NewPN, if they are contained
// in the region.
+ for (unsigned i = 0; i != PN->getNumIncomingValues(); ++i) {
+ BasicBlock *HasValToPN = PN->getIncomingBlock(i);
+ if (R->contains(HasValToPN)) {
+ NewPN->addIncoming(PN->getIncomingValue(i), HasValToPN);
+ PN->removeIncomingValue(i);
+ --i;
+ }
+ }
+ }
+
+ return newEntry;
+}
> +}
> +
> +// createSingleExitEdge - Split the exit basic of the given region
^
block
> +// 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);
We can add an assert here to check if we need to split at all:
assert(Preds.size() && "This region has already a single exit edge");
> +
> + BasicBlock *newExit = SplitBlockPredecessors(oldExit,
> + Preds.data(), Preds.size(),
> + ".single_exit", this);
> + RegionInfo *RI =&getAnalysis<RegionInfo> ();
Please remove the space before the '()'.
> +
> + // 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.
Can you add:
// Children of this region cannot have an entry node that matches
// the exiting node of this region.
> + // 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
// If the exit of a descendant of RI is oldExit, change it to
// newExit.
Maybe this is a little clearer.
> + std::vector<Region *> RQ;
> + for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE; ++RI)
> + RQ.push_back(*RI);
> +
> + while (!RQ.empty()){
> + R = RQ.back();
> + RQ.pop_back();
The indentation is here just one space. It should be two.
> +
> + for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE; ++RI)
> + RQ.push_back(*RI);
> +
> + if (R->getExit() == oldExit)
> + R->replaceExit(newExit);
> + }
I believe it is correct to optimize this to:
while (!RQ.empty()) {
R = RQ.back();
RQ.pop_back();
if (R->getExit() != oldExit)
continue;
for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE;
++RI)
RQ.push_back(*RI);
R->replaceExit(newExit);
}
This passes all test cases. In case this is incorrect, please add a test
case that shows it is incorrect.
> +
> +}
> +
> +bool RegionSimplify::runOnRegion(Region *R, RGPassManager&RGM) {
> + modified = false;
> +
> + if (!R->isTopLevelRegion()) {
Can you change this to:
if (R->isTopLevelRegion)
return false;
And reduce the indentation.
> + // split entry node if the region has multiple entry edges
// The region has multiple exits.
Comments start with a large letter and finish with a '.' or something
equivalent. I also propose to remove the part 'Split entry node' as this
information is nicely available in the name of the function called
afterwords.
> + if (!(R->getEnteringBlock())
> +&& (pred_begin(R->getEntry()) != pred_end(R->getEntry()))) {
> + createSingleEntryEdge(R);
> + modified = true;
> + ++NumEntries;
> + }
> +
> + // The region has multiple exit edges.
> + if (!(R->getExitingBlock())) {
> + createSingleExitEdge(R);
> + modified = true;
> + ++NumExits;
> + }
> + }
> +
> + 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/basic_test.ll b/test/Transforms/RegionSimplify/basic_test.ll
> new file mode 100644
> index 0000000..e51772f
> --- /dev/null
> +++ b/test/Transforms/RegionSimplify/basic_test.ll
> @@ -0,0 +1,28 @@
> +; RUN: opt< %s -region-simplify -S | FileCheck %s
Very nice test cases. Just one thing. Can you add -verify-region-info to
the flags of all tests? (I tried it and it works).
Tobi
More information about the llvm-commits
mailing list