Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG #7268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG #7268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions compiler/infra/OMRCfg.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ class CFG
_calledFrequency = 0;
_initialBlockFrequency = -1;
_edgeProbabilities = NULL;
_hasBackEdges = false;
}

TR::CFG * self();
Expand Down Expand Up @@ -360,6 +361,9 @@ class CFG

static const char *blockFrequencyNames[];

void setHasBackEdges() { _hasBackEdges = true; }
bool hasBackEdges() { return _hasBackEdges; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This state isn't meaningfully set or used in this PR - I assume it's used in eclipse-openj9/openj9#17813

Does this need to be on the CFG? Or could it be kept somewhere else instead? e.g. next to a CFG pointer in the code that cares about it. I'm just worried that it will easily get out of date. In fact, with the fixed initial value (false), it will immediately be out of date for many CFGs. (And that would still be the case if the fixed initial value were true instead.)


protected:
TR::Compilation *_compilation;
TR::ResolvedMethodSymbol *_method;
Expand All @@ -379,6 +383,7 @@ class CFG
bool _ignoreUnreachableBlocks;
bool _removingUnreachableBlocks;

bool _hasBackEdges;

TR::CFGNode **_forwardTraversalOrder;
int32_t _forwardTraversalLength;
Expand Down
35 changes: 28 additions & 7 deletions compiler/optimizer/Dominators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,32 @@ TR_Dominators::TR_Dominators(TR::Compilation *c, bool post) :
_info(c->getFlowGraph()->getNextNodeNumber()+1, BBInfo(_region), _region),
_dfNumbers(c->getFlowGraph()->getNextNodeNumber()+1, 0, _region),
_dominators(c->getFlowGraph()->getNextNodeNumber()+1, static_cast<TR::Block *>(NULL), _region)
{
construct(c->getFlowGraph(), post);
}

TR_Dominators::TR_Dominators(TR::Compilation *c, TR::CFG* cfg, bool acceptUnreachableBlocks, bool post) :
_region(c->trMemory()->heapMemoryRegion()),
_compilation(c),
_info(cfg->getNextNodeNumber()+1, BBInfo(_region), _region),
_dfNumbers(cfg->getNextNodeNumber()+1, 0, _region),
_dominators(cfg->getNextNodeNumber()+1, static_cast<TR::Block *>(NULL), _region)
{
construct(cfg, post, acceptUnreachableBlocks);
}

void TR_Dominators::construct(TR::CFG* cfg, bool post, bool acceptUnreachableBlocks)
{
LexicalTimer tlex("TR_Dominators::TR_Dominators", _compilation->phaseTimer());

_postDominators = post;
_isValid = true;
_hasUnreachableBlocks = false;
_topDfNum = 0;
_visitCount = c->incOrResetVisitCount();
_visitCount = _compilation->incOrResetVisitCount();
_trace = comp()->getOption(TR_TraceDominators);

TR::CFG *cfg = c->getFlowGraph();

_cfg = c->getFlowGraph();
_cfg = cfg;
_numNodes = cfg->getNumberOfNodes()+1;

if (trace())
Expand All @@ -80,7 +94,7 @@ TR_Dominators::TR_Dominators(TR::Compilation *c, bool post) :
_dominators[dominated->getNumber()] = dominator;
if (trace())
traceMsg(comp(), " %sDominator of block_%d is block_%d\n", _postDominators ? "post-" : "",
dominated->getNumber(), dominator->getNumber());
dominated->getNumber(), dominator->getNumber());
}

// The exit block may not be reachable from the entry node. In this case just
Expand Down Expand Up @@ -110,15 +124,22 @@ TR_Dominators::TR_Dominators(TR::Compilation *c, bool post) :
traceMsg(comp(), "Some blocks are not reachable from exit. Post-dominator info is invalid.\n");
return;
}
if (!acceptUnreachableBlocks)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the name acceptUnreachableBlocks is misleading - it's almost exactly backwards. The assertion isn't there to accept/ignore unreachable blocks. Rather, it's there to crash if it finds one

It might be better to have the name describe the input, e.g. what do you think of cfgMayContainUnreachableBlocks? If that's true, then we have to deal with the unreachable blocks (by indicating that they were found), and otherwise we can fail the assertion because they aren't supposed to be there

{
_hasUnreachableBlocks = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please stick with _isValid, which already has the same meaning, and remove _hasUnreachableBlocks

if (trace())
traceMsg(comp(), "Unreachable block in the CFG %d %d\n", _topDfNum, _numNodes-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this message mention that the dominators are therefore invalid, like in the postdominator case?

return;
}
else
TR_ASSERT(false, "Unreachable block in the CFG %d %d", _topDfNum, _numNodes-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a TODO comment for changing this to TR_ASSERT_FATAL?

}

#if DEBUG
for (auto block = toBlock(cfg->getFirstNode()); block; block = toBlock(block->getNext()))
{
{
TR_ASSERT(_dfNumbers[block->getNumber()] >= 0, "Unreachable block in the CFG");
}
}
#endif

if (trace())
Expand Down
6 changes: 6 additions & 0 deletions compiler/optimizer/Dominators.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,14 @@ class TR_Dominators
TR_ALLOC(TR_Memory::Dominators)

TR_Dominators(TR::Compilation *, bool post = false);
TR_Dominators(TR::Compilation *, TR::CFG* cfg, bool acceptUnreachableBlocks = true, bool post = false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does acceptUnreachableBlocks need to be a parameter? I expect TR_RegionAnalysis::getRegions(...cfg...) to be the only use of this. It's a parameter there as well, but I think it probably doesn't need to be

TR::Block *getDominator(TR::Block *);
int dominates(TR::Block *block, TR::Block *other);

TR::Compilation * comp() { return _compilation; }
bool trace() { return _trace; }
bool isValid() { return _isValid; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment to isValid() that explains why it's now possible for this to be invalid? e.g. something like this:

   // HACK: Dominators should always be valid, but for some reason the benefit
   // inliner can pass a CFG with unreachable blocks.

bool hasUnreachableBlocks() { return _hasUnreachableBlocks; }

protected:

Expand Down Expand Up @@ -113,6 +116,8 @@ class TR_Dominators
int32_t parent;
};

void construct(TR::CFG* cfg, bool post, bool acceptUnreachableBlocks = true);

BBInfo& getInfo(int32_t index) {return _info[index];}
int32_t blockNumber(int32_t index) {return _info[index]._block->getNumber();}

Expand All @@ -133,6 +138,7 @@ class TR_Dominators
int32_t _numNodes;
int32_t _topDfNum;
vcount_t _visitCount;
bool _hasUnreachableBlocks;

protected:
TR::CFG * _cfg;
Expand Down
70 changes: 28 additions & 42 deletions compiler/optimizer/StructuralAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,34 +171,28 @@ void TR_RegionAnalysis::createLeafStructures(TR::CFG *cfg, TR::Region &region)
/**
* Mainline for performing Region Analysis.
*/
TR_Structure *TR_RegionAnalysis::getRegions(TR::Compilation *comp, TR::ResolvedMethodSymbol* methSym)
TR_Structure *TR_RegionAnalysis::getRegionsImpl(TR::Compilation *comp, TR_Dominators& dominators, TR::CFG* cfg)
{
TR::StackMemoryRegion stackMemoryRegion(*comp->trMemory());

// Calculate dominators
// This has the side-effect of renumbering the blocks in depth-first order
//
TR_Dominators dominators = TR_Dominators(comp);

#if DEBUG
if (debug("verifyDominator"))
{
{
TR_DominatorVerifier verifyDominator(dominators);
}
}
#endif

TR::CFG *cfg = methSym->getFlowGraph();
TR_ASSERT(cfg, "cfg is NULL\n");

TR_RegionAnalysis ra(comp, dominators, cfg, stackMemoryRegion);
ra._trace = comp->getOption(TR_TraceSA);

ra._useNew = !comp->getOption(TR_DisableIterativeSA);
if (ra.trace())
{
{
traceMsg(comp, "Blocks before Region Analysis:\n");
comp->getDebug()->print(comp->getOutFile(), cfg);
}
}

ra.createLeafStructures(cfg, stackMemoryRegion);

Expand All @@ -210,48 +204,40 @@ TR_Structure *TR_RegionAnalysis::getRegions(TR::Compilation *comp, TR::ResolvedM
return result;
}

TR_Structure *TR_RegionAnalysis::getRegions(TR::Compilation *comp, TR::CFG* cfg, bool acceptUnreachableBlocks)
{
// Calculate dominators
// This has the side-effect of renumbering the blocks in depth-first order
//
// If acceptUnreachableBlocks is false, dominators constructor will early return
//
TR_Dominators dominators(comp, cfg, acceptUnreachableBlocks);
if (!acceptUnreachableBlocks && dominators.hasUnreachableBlocks())
return NULL;

return getRegionsImpl(comp, dominators, cfg);
}


/**
* Mainline for performing Region Analysis.
*/
TR_Structure *TR_RegionAnalysis::getRegions(TR::Compilation *comp)
TR_Structure *TR_RegionAnalysis::getRegions(TR::Compilation *comp, TR::ResolvedMethodSymbol* methSym)
{
TR::StackMemoryRegion stackMemoryRegion(*comp->trMemory());

// Calculate dominators
// This has the side-effect of renumbering the blocks in depth-first order
//
TR_Dominators dominators(comp);
TR::CFG *cfg = methSym->getFlowGraph();

#if DEBUG
if (debug("verifyDominator"))
{
TR_DominatorVerifier verifyDominator(dominators);
}
#endif

TR::CFG *cfg = comp->getFlowGraph();

TR_RegionAnalysis ra(comp, dominators, cfg, stackMemoryRegion);
ra._trace = comp->getOption(TR_TraceSA);

ra._useNew = !comp->getOption(TR_DisableIterativeSA);
if (ra.trace())
{
traceMsg(comp, "Blocks before Region Analysis:\n");
comp->getDebug()->print(comp->getOutFile(), cfg);
}

ra.createLeafStructures(cfg, stackMemoryRegion);
return getRegionsImpl(comp, dominators, cfg);
}

// Loop through the node set until there is only one node left - this is the
// root of the control tree.
TR_Structure *TR_RegionAnalysis::getRegions(TR::Compilation *comp)
{
// Calculate dominators
// This has the side-effect of renumbering the blocks in depth-first order
//
TR_Structure *result = ra.findRegions(stackMemoryRegion);
TR_Dominators dominators(comp);
TR::CFG *cfg = comp->getFlowGraph();

return result;
return getRegionsImpl(comp, dominators, cfg);
}

/**
Expand Down
3 changes: 3 additions & 0 deletions compiler/optimizer/StructuralAnalysis.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class TR_RegionAnalysis

static TR_Structure *getRegions(TR::Compilation *);
static TR_Structure *getRegions(TR::Compilation *, TR::ResolvedMethodSymbol *);
static TR_Structure *getRegions(TR::Compilation *, TR::CFG *, bool acceptUnreachableBlocks = true);

friend class TR_Debug;

Expand Down Expand Up @@ -122,6 +123,8 @@ class TR_RegionAnalysis
bool _useNew;
void createLeafStructures(TR::CFG *cfg, TR::Region &region);

static TR_Structure *getRegionsImpl(TR::Compilation *comp, TR_Dominators& dominators, TR::CFG* cfg);

TR_Structure *findRegions(TR::Region &region);
TR_RegionStructure *findNaturalLoop(StructInfo &node,
WorkBitVector &regionNodes,
Expand Down