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

Remove unsafe expressions after store sinking #7573

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove unsafe expressions after store sinking #7573

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba02e80
Enable dumping of method trees in Store Sinking
hzongaro Nov 12, 2024
d9735f5
Move UnsafeSubexpressionRemoval to its own file
hzongaro Nov 14, 2024
ce9af00
Remove unused nextStoreWasMoved parameter of sinkStorePlacement
hzongaro Nov 21, 2024
4e68ec7
Handle unsafe loads in trees left behind by Store Sinking
hzongaro Nov 25, 2024
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
1 change: 1 addition & 0 deletions compiler/optimizer/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ compiler_library(optimizer
${CMAKE_CURRENT_LIST_DIR}/TrivialDeadBlockRemover.cpp
${CMAKE_CURRENT_LIST_DIR}/FEInliner.cpp
${CMAKE_CURRENT_LIST_DIR}/BenefitInliner.cpp
${CMAKE_CURRENT_LIST_DIR}/UnsafeSubexpressionRemover.cpp
${CMAKE_CURRENT_LIST_DIR}/abstractinterpreter/AbsValue.cpp
${CMAKE_CURRENT_LIST_DIR}/abstractinterpreter/AbsOpStack.cpp
${CMAKE_CURRENT_LIST_DIR}/abstractinterpreter/AbsOpArray.cpp
Expand Down
200 changes: 3 additions & 197 deletions compiler/optimizer/IsolatedStoreElimination.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
#include "optimizer/Optimizer.hpp"
#include "optimizer/Structure.hpp"
#include "optimizer/TransformUtil.hpp"
#include "optimizer/UnsafeSubexpressionRemover.hpp"
#include "optimizer/UseDefInfo.hpp"
#include "optimizer/VPConstraint.hpp"
#include "ras/Debug.hpp"
Expand All @@ -77,118 +78,6 @@ TR_IsolatedStoreElimination::TR_IsolatedStoreElimination(TR::OptimizationManager
{
}

struct UnsafeSubexpressionRemoval // PR69613
{
TR_IsolatedStoreElimination *_ise;
TR_BitVector _visitedNodes;
TR_BitVector _unsafeNodes; // Nodes whose mere evaluation is not safe

TR::Compilation *comp(){ return _ise->comp(); }
bool trace(){ return _ise->trace(); }

bool isVisited(TR::Node *node) { return _visitedNodes.isSet(node->getGlobalIndex()); }
bool isUnsafe(TR::Node *node)
{
TR_ASSERT(isVisited(node), "Cannot call isUnsafe on n%dn before anchorSafeChildrenOfUnsafeNodes", node->getGlobalIndex());
return _unsafeNodes.isSet(node->getGlobalIndex());
}

UnsafeSubexpressionRemoval(TR_IsolatedStoreElimination *ise):
_ise(ise),
_visitedNodes(ise->comp()->getNodeCount(), comp()->trMemory(), stackAlloc),
_unsafeNodes (ise->comp()->getNodeCount(), comp()->trMemory(), stackAlloc)
{}

void recordDeadUse(TR::Node *node)
{
_visitedNodes.set(node->getGlobalIndex());
_unsafeNodes .set(node->getGlobalIndex());
}

void anchorSafeChildrenOfUnsafeNodes(TR::Node *node, TR::TreeTop *anchorPoint)
{
if (isVisited(node))
return;
else
_visitedNodes.set(node->getGlobalIndex());

//
// Design note: we don't decrement refcounts in here. Conceptually,
// anchoring and decrementing are independent of each other. More
// pragmatically, you end up at with a well-defined state when this
// function finishes: if the top-level node is unsafe, the caller must
// unhook it and call recursivelyDecReferenceCount, which will do the
// right thing. Otherwise, no decrementing is necessary anyway.
//
// A rule of thumb to remember is that this function will do nothing if
// the node is safe. If you call this, then find !isUnsafe(node), then
// the trees did not change.
//

// Propagate unsafeness from children, unless we've already done it
//
for (int32_t i = 0; i < node->getNumChildren(); ++i)
{
TR::Node *child = node->getChild(i);
anchorSafeChildrenOfUnsafeNodes(child, anchorPoint);
if (isUnsafe(child))
{
_unsafeNodes.set(node->getGlobalIndex());
if (trace())
{
TR::Node *child = node->getChild(i);
traceMsg(comp(), " (Marked %s n%dn unsafe due to dead child #%d %s n%dn)\n",
node->getOpCode().getName(), node->getGlobalIndex(), i,
child->getOpCode().getName(), child->getGlobalIndex());
}
}
}

// If node is safe, all its descendant nodes must be safe. Nothing to do.
//
// If node is unsafe, anchor all its safe children. Its unsafe children,
// at this point, must already have taken care of their own safe
// subexpressions recursively.
//
// Note that this could anchor some nodes multiple times, for complex
// subexpressions with commoning. We could make some feeble attempts to
// prevent that, but let's just leave it to deadTreesElimination.
//
if (isUnsafe(node))
{
for (int32_t i = 0; i < node->getNumChildren(); ++i)
{
TR::Node *child = node->getChild(i);
bool didIt = anchorIfSafe(child, anchorPoint);
if (didIt && trace())
{
traceMsg(comp(), " - Anchored child #%d %s n%d of %s n%d\n",
i,
child->getOpCode().getName(), child->getGlobalIndex(),
node->getOpCode().getName(), node->getGlobalIndex());
}
}
}
}

bool anchorIfSafe(TR::Node *node, TR::TreeTop *anchorPoint)
{
if (!isVisited(node))
anchorSafeChildrenOfUnsafeNodes(node, anchorPoint);

if (isUnsafe(node))
{
return false;
}
else
{
anchorPoint->insertBefore(TR::TreeTop::create(comp(), TR::Node::create(TR::treetop, 1, node)));
return true;
}
}

};

int32_t TR_IsolatedStoreElimination::perform()
{
TR::StackMemoryRegion stackMemoryRegion(*trMemory());
Expand Down Expand Up @@ -233,7 +122,7 @@ int32_t TR_IsolatedStoreElimination::perform()
// example, if the unsafe node dereferences a load of a dead pointer
// whose store got deleted, then that dereference could crash at runtime.
//
UnsafeSubexpressionRemoval usr(this);
OMR::UnsafeSubexpressionRemover usr(this);
for (uint32_t groupIndex = 0; groupIndex < _groupsOfStoreNodes->size(); groupIndex++)
{
TR_BitVector *groupOfStores = _groupsOfStoreNodes->element(groupIndex);
Expand Down Expand Up @@ -291,90 +180,7 @@ int32_t TR_IsolatedStoreElimination::perform()

_trivialDefs->reset(index); // It's not a trivial def if we're handling it as a group. If we try to do both, BOOM!

// Time to delete the store. This is more complex than it sounds.
//
// Generally, we want to replace the store with a treetop (ie. a
// no-op). However, this is complicated by several factors, in
// order of increasing subtlety and complexity:
//
// 1. Stores can have multiple children -- as many as three for
// an iwrtbar. If the store is turning into a nop, we must
// anchor all the other children.
// 2. Stores can appear under other nodes like CHKs and
// compressedRefs. Particularly for the CHKs, we must
// perform the appropriate checks despite eliminating the
// store, and so we must leave the store node in the
// appropriate shape so its parents will do the right thing.
// 3. Some expressions are unsafe to evaluate after stores are
// eliminated, and those expressions themselves must NOT be
// anchored.
//
// In these transformations, we adopt the convention that we
// retain the original store whenever possible, modifying it
// in-place, and we retain its first child if possible. These
// conventions are sometimes required, and so we just adopt them
// universally for the sake of consistency, efficiency, and
// readability in the jit logs.

// Anchor all children but the first
//
for (int32_t i = 1; i < node->getNumChildren(); i++)
{
TR::Node *child = node->getChild(i);
usr.anchorIfSafe(child, treeTop);
child->recursivelyDecReferenceCount();
}

node->setNumChildren(1);
//
// Note that the node at this point could be half-baked: an indirect
// store or write barrier with one child. This is an intermediate
// state, and further surgery is required for correctness.

// Eliminate the store
//
if (treeTop->getNode()->getOpCode().isSpineCheck() && treeTop->getNode()->getFirstChild() == node)
{
// SpineCHKs are special. We don't want to require all the
// opts and codegens to do the right thing with a PassThrough
// child. Hence, we'll turn the tree into a const, which is
// a state the opts and codegens must cope with anyway,
// because if this tree had been a load instead of a store,
// the optimizer could have legitimately turned it into a const.
//
TR::Node *child = node->getChild(0);
usr.anchorIfSafe(child, treeTop);
child->recursivelyDecReferenceCount();
TR::Node::recreate(node, comp()->il.opCodeForConst(node->getSymbolReference()->getSymbol()->getDataType()));
node->setFlags(0);
node->setNumChildren(0);
}
else
{
// Vandalize the first child if it's unsafe to evaluate
//
TR::Node *child = node->getFirstChild();
usr.anchorSafeChildrenOfUnsafeNodes(child, treeTop);
if (usr.isUnsafe(child))
{
// Must eradicate the dead expression. Can't leave it
// anchored because it's not safe to evaluate.
//
child->recursivelyDecReferenceCount();
TR::Node *dummyChild = node->setAndIncChild(0, TR::Node::createConstDead(child, TR::Int32, 0xbad1 /* eyecatcher */));
if (trace())
traceMsg(comp(), " - replace unsafe child %s n%dn with dummy %s n%dn\n",
child->getOpCode().getName(), child->getGlobalIndex(),
dummyChild->getOpCode().getName(), dummyChild->getGlobalIndex());
}

// Set opcode to nop
//
if (node->getReferenceCount() >= 1)
TR::Node::recreate(node, TR::PassThrough);
else
TR::Node::recreate(node, TR::treetop);
}
usr.eliminateStore(treeTop);

node->setFlags(0);
eliminatedStore = true;
Expand Down
5 changes: 3 additions & 2 deletions compiler/optimizer/SinkStores.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
#include "optimizer/Optimizations.hpp"
#include "optimizer/Optimizer.hpp"
#include "optimizer/Structure.hpp"
#include "optimizer/UnsafeSubexpressionRemover.hpp"

#define OPT_DETAILS "O^O SINK STORES: "

Expand Down Expand Up @@ -516,7 +517,7 @@ int32_t TR_GeneralSinkStores::perform()

int32_t TR_SinkStores::performStoreSinking()
{
if (0 && trace())
if (trace())
{
comp()->dumpMethodTrees("Before Store Sinking");
}
Expand Down Expand Up @@ -659,7 +660,7 @@ int32_t TR_SinkStores::performStoreSinking()
// if we found any, transform the code now
doSinking();

if (0 && trace())
if (trace())
{
comp()->dumpMethodTrees("After Store Sinking");
}
Expand Down
1 change: 1 addition & 0 deletions compiler/optimizer/SinkStores.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class TR_LiveVariableInformation;
class TR_Liveness;
class TR_MovableStore;
class TR_SinkStores;
namespace OMR { struct UnsafeSubexpressionRemover; }
namespace TR { class Block; }
namespace TR { class CFGEdge; }
namespace TR { class CFGNode; }
Expand Down
Loading