Skip to content

Commit

Permalink
[GR-34764] Add some sanity checks around the use of init memory
Browse files Browse the repository at this point in the history 
PullRequest: graal/10202
  • Loading branch information
@tkrodriguez
tkrodriguez committed Nov 4, 2021
2 parents 227100f + 4b36166 commit 2496739
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 9 deletions.
7 changes: 5 additions & 2 deletions ....graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/extended/FixedValueAnchorNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@
import org.graalvm.compiler.core.common.type.StampFactory;
import org.graalvm.compiler.graph.Node;
import org.graalvm.compiler.graph.NodeClass;
import org.graalvm.compiler.nodes.spi.Canonicalizable;
import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
import org.graalvm.compiler.nodeinfo.NodeInfo;
import org.graalvm.compiler.nodes.FixedWithNextNode;
import org.graalvm.compiler.nodes.NodeView;
import org.graalvm.compiler.nodes.ValueNode;
import org.graalvm.compiler.nodes.spi.Canonicalizable;
import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
import org.graalvm.compiler.nodes.spi.LIRLowerable;
import org.graalvm.compiler.nodes.spi.NodeLIRBuilderTool;
import org.graalvm.compiler.nodes.spi.ValueProxy;
Expand Down Expand Up @@ -81,6 +81,9 @@ public Node canonical(CanonicalizerTool tool) {
if (tool.allUsagesAvailable() && hasNoUsages()) {
return null;
}
if (object instanceof FixedValueAnchorNode && ((FixedValueAnchorNode) object).next == this) {
return object;
}
return this;
}

Expand Down
37 changes: 30 additions & 7 deletions ...alvm.compiler.phases.common/src/org/graalvm/compiler/phases/common/FloatingReadPhase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import static org.graalvm.compiler.graph.Graph.NodeEvent.NODE_ADDED;
import static org.graalvm.compiler.graph.Graph.NodeEvent.ZERO_USAGES;
import static org.graalvm.word.LocationIdentity.any;
import static org.graalvm.word.LocationIdentity.init;

import java.util.EnumSet;
import java.util.Iterator;
Expand All @@ -38,6 +39,7 @@
import org.graalvm.collections.UnmodifiableMapCursor;
import org.graalvm.compiler.core.common.cfg.Loop;
import org.graalvm.compiler.debug.DebugCloseable;
import org.graalvm.compiler.debug.GraalError;
import org.graalvm.compiler.graph.Graph.NodeEventScope;
import org.graalvm.compiler.graph.Node;
import org.graalvm.compiler.nodes.AbstractBeginNode;
Expand All @@ -51,13 +53,15 @@
import org.graalvm.compiler.nodes.ReturnNode;
import org.graalvm.compiler.nodes.StartNode;
import org.graalvm.compiler.nodes.StructuredGraph;
import org.graalvm.compiler.nodes.StructuredGraph.StageFlag;
import org.graalvm.compiler.nodes.ValueNode;
import org.graalvm.compiler.nodes.ValueNodeUtil;
import org.graalvm.compiler.nodes.WithExceptionNode;
import org.graalvm.compiler.nodes.StructuredGraph.StageFlag;
import org.graalvm.compiler.nodes.calc.FloatingNode;
import org.graalvm.compiler.nodes.cfg.Block;
import org.graalvm.compiler.nodes.cfg.ControlFlowGraph;
import org.graalvm.compiler.nodes.cfg.HIRLoop;
import org.graalvm.compiler.nodes.memory.AddressableMemoryAccess;
import org.graalvm.compiler.nodes.memory.FloatableAccessNode;
import org.graalvm.compiler.nodes.memory.FloatingAccessNode;
import org.graalvm.compiler.nodes.memory.FloatingReadNode;
Expand All @@ -70,6 +74,7 @@
import org.graalvm.compiler.nodes.memory.MultiMemoryKill;
import org.graalvm.compiler.nodes.memory.ReadNode;
import org.graalvm.compiler.nodes.memory.SingleMemoryKill;
import org.graalvm.compiler.nodes.memory.address.AddressNode;
import org.graalvm.compiler.nodes.util.GraphUtil;
import org.graalvm.compiler.phases.Phase;
import org.graalvm.compiler.phases.common.util.EconomicSetNodeEventListener;
Expand Down Expand Up @@ -214,6 +219,8 @@ private EconomicSet<LocationIdentity> processLoop(HIRLoop loop, EconomicMap<Loop
@Override
@SuppressWarnings("try")
protected void run(StructuredGraph graph) {
EconomicSet<ValueNode> initMemory = EconomicSet.create(Equivalence.IDENTITY);

EconomicMap<LoopBeginNode, EconomicSet<LocationIdentity>> modifiedInLoops = null;
if (graph.hasLoops()) {
modifiedInLoops = EconomicMap.create(Equivalence.IDENTITY);
Expand All @@ -226,7 +233,7 @@ protected void run(StructuredGraph graph) {

EconomicSetNodeEventListener listener = new EconomicSetNodeEventListener(EnumSet.of(NODE_ADDED, ZERO_USAGES));
try (NodeEventScope nes = graph.trackNodeEvents(listener)) {
ReentrantNodeIterator.apply(new FloatingReadClosure(modifiedInLoops, createFloatingReads, createMemoryMapNodes), graph.start(), new MemoryMapImpl(graph.start()));
ReentrantNodeIterator.apply(new FloatingReadClosure(modifiedInLoops, createFloatingReads, createMemoryMapNodes, initMemory), graph.start(), new MemoryMapImpl(graph.start()));
}

for (Node n : removeExternallyUsedNodes(listener.getNodes())) {
Expand Down Expand Up @@ -299,11 +306,14 @@ public static class FloatingReadClosure extends NodeIteratorClosure<MemoryMapImp
private final EconomicMap<LoopBeginNode, EconomicSet<LocationIdentity>> modifiedInLoops;
private boolean createFloatingReads;
private boolean createMemoryMapNodes;
private final EconomicSet<ValueNode> initMemory;

public FloatingReadClosure(EconomicMap<LoopBeginNode, EconomicSet<LocationIdentity>> modifiedInLoops, boolean createFloatingReads, boolean createMemoryMapNodes) {
public FloatingReadClosure(EconomicMap<LoopBeginNode, EconomicSet<LocationIdentity>> modifiedInLoops, boolean createFloatingReads, boolean createMemoryMapNodes,
EconomicSet<ValueNode> initMemory) {
this.modifiedInLoops = modifiedInLoops;
this.createFloatingReads = createFloatingReads;
this.createMemoryMapNodes = createMemoryMapNodes;
this.initMemory = initMemory;
}

@Override
Expand Down Expand Up @@ -372,30 +382,43 @@ private static void processAccess(MemoryAccess access, MemoryMapImpl state) {
}
}

private static void processCheckpoint(SingleMemoryKill checkpoint, MemoryMapImpl state) {
private void processCheckpoint(SingleMemoryKill checkpoint, MemoryMapImpl state) {
processIdentity(checkpoint.getKilledLocationIdentity(), checkpoint, state);
}

private static void processCheckpoint(MultiMemoryKill checkpoint, MemoryMapImpl state) {
private void processCheckpoint(MultiMemoryKill checkpoint, MemoryMapImpl state) {
for (LocationIdentity identity : checkpoint.getKilledLocationIdentities()) {
processIdentity(identity, checkpoint, state);
}
}

private static void processIdentity(LocationIdentity identity, MemoryKill checkpoint, MemoryMapImpl state) {
private void processIdentity(LocationIdentity identity, MemoryKill checkpoint, MemoryMapImpl state) {
if (identity.isAny()) {
state.getMap().clear();
}
if (identity.isMutable()) {
state.getMap().put(identity, checkpoint);
}

if (checkpoint instanceof AddressableMemoryAccess) {
AddressNode address = ((AddressableMemoryAccess) checkpoint).getAddress();
if (identity.equals(init())) {
// Track bases which are used for init memory
initMemory.add(address.getBase());
}
}
}

@SuppressWarnings("try")
private static void processFloatable(FloatableAccessNode accessNode, MemoryMapImpl state) {
private void processFloatable(FloatableAccessNode accessNode, MemoryMapImpl state) {
StructuredGraph graph = accessNode.graph();
LocationIdentity locationIdentity = accessNode.getLocationIdentity();

if (accessNode.canFloat()) {
// Bases can't be used for both init and other memory locations since init doesn't
// participate in the memory graph.
GraalError.guarantee(!initMemory.contains(accessNode.getAddress().getBase()), "base used for init cannot be used for other accesses: %s", accessNode);

assert accessNode.getNullCheck() == false;
MemoryKill lastLocationAccess = state.getLastLocationAccess(locationIdentity);
try (DebugCloseable position = accessNode.withNodeSourcePosition()) {
Expand Down

0 comments on commit 2496739

Please sign in to comment.