8280568: IGV: Phi inputs and pinned nodes are not scheduled correctly

Reviewed-by: kvn, thartmann
This commit is contained in:
Roberto Castañeda Lozano 2022-05-03 07:27:50 +00:00
parent 64b5b2b0b3
commit 7a4835178d
8 changed files with 408 additions and 47 deletions

View File

@ -0,0 +1,77 @@
/*
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/
package com.sun.hotspot.igv.filter;
import com.sun.hotspot.igv.data.Properties;
import com.sun.hotspot.igv.graph.*;
import java.util.ArrayList;
import java.util.List;
public class WarningFilter extends AbstractFilter {
private final List<WarningRule> rules;
private final String name;
private final String warning;
public WarningFilter(String name, String warning) {
this.name = name;
this.warning = warning;
rules = new ArrayList<>();
}
@Override
public String getName() {
return name;
}
@Override
public void apply(Diagram diagram) {
Properties.PropertySelector<Figure> selector = new Properties.PropertySelector<>(diagram.getFigures());
for (WarningRule rule : rules) {
if (rule.getSelector() != null) {
List<Figure> figures = rule.getSelector().selected(diagram);
for (Figure f : figures) {
f.setWarning(warning);
}
}
}
}
public void addRule(WarningRule r) {
rules.add(r);
}
public static class WarningRule {
private Selector selector;
public WarningRule(Selector selector) {
this.selector = selector;
}
public Selector getSelector() {
return selector;
}
}
}

View File

@ -1,5 +1,5 @@
/*
* Copyright (c) 2008, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2008, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@ -33,6 +33,12 @@ function colorize(property, regexp, color) {
f.apply(graph);
}
function warn(propertyToMatch, regexp, propertyToShow) {
var f = new WarningFilter("", "[" + propertyToShow + "]");
f.addRule(new WarningFilter.WarningRule(new MatcherSelector(new Properties.RegexpPropertyMatcher(propertyToMatch, regexp))));
f.apply(graph);
}
function remove(property, regexp) {
var f = new RemoveFilter("");
f.addRule(new RemoveFilter.RemoveRule(new MatcherSelector(new Properties.RegexpPropertyMatcher(property, regexp))));
@ -111,4 +117,4 @@ var orange = Color.orange;
var pink = Color.pink
var red = Color.red;
var yellow = Color.yellow;
var white = Color.white;
var white = Color.white;

View File

@ -43,6 +43,7 @@ public class Figure extends Properties.Entity implements Source.Provider, Vertex
public static final int SLOT_OFFSET = 8;
public static final int TOP_CFG_HEIGHT = 7;
public static final int BOTTOM_CFG_HEIGHT = 6;
public static final int WARNING_WIDTH = 16;
protected List<InputSlot> inputSlots;
protected List<OutputSlot> outputSlots;
private Source source;
@ -52,6 +53,7 @@ public class Figure extends Properties.Entity implements Source.Provider, Vertex
private List<Figure> successors;
private List<InputGraph> subgraphs;
private Color color;
private String warning;
private int id;
private String idString;
private String[] lines;
@ -123,6 +125,9 @@ public class Figure extends Properties.Entity implements Source.Provider, Vertex
}
}
widthCash = max + INSET;
if (getWarning() != null) {
widthCash += WARNING_WIDTH;
}
widthCash = Math.max(widthCash, Figure.getSlotsWidth(inputSlots));
widthCash = Math.max(widthCash, Figure.getSlotsWidth(outputSlots));
}
@ -155,6 +160,14 @@ public class Figure extends Properties.Entity implements Source.Provider, Vertex
return color;
}
public void setWarning(String warning) {
this.warning = getProperties().resolveString(warning);
}
public String getWarning() {
return warning;
}
public boolean hasInputList() {
return diagram.isCFG() && !getPredecessors().isEmpty();
}

View File

@ -46,15 +46,30 @@ public class ServerCompilerScheduler implements Scheduler {
private static class Node {
public static final String WARNING_BLOCK_PROJECTION_WITH_MULTIPLE_SUCCS = "Block projection with multiple successors";
public static final String WARNING_PHI_INPUT_WITHOUT_REGION = "Phi input without associated region";
public static final String WARNING_REGION_WITHOUT_CONTROL_INPUT = "Region without control input";
public static final String WARNING_PHI_WITH_REGIONLESS_INPUTS = "Phi node with input nodes without associated region";
public static final String WARNING_NOT_MARKED_WITH_BLOCK_START = "Region not marked with is_block_start";
public static final String WARNING_CFG_AND_INPUT_TO_PHI = "CFG node is a phi input";
public static final String WARNING_PHI_NON_DOMINATING_INPUTS = "Phi input that does not dominate the phi's input block";
public InputNode inputNode;
public Set<Node> succs = new HashSet<>();
public List<Node> preds = new ArrayList<>();
// Index of each predecessor.
public List<Character> predIndices = new ArrayList<>();
public List<String> warnings;
public InputBlock block;
public boolean isBlockProjection;
public boolean isBlockStart;
public boolean isCFG;
public int rank; // Rank for local scheduling priority.
// Empty constructor for creating dummy CFG nodes without associated
// input nodes.
public Node() {}
public Node(InputNode n) {
inputNode = n;
String p = n.getProperties().get("is_block_proj");
@ -81,8 +96,18 @@ public class ServerCompilerScheduler implements Scheduler {
}
}
public void addWarning(String msg) {
if (warnings == null) {
warnings = new ArrayList<>();
}
warnings.add(msg);
}
@Override
public String toString() {
if (inputNode == null) {
return "(dummy node)";
}
return inputNode.getProperties().get("idx") + " " + inputNode.getProperties().get("name");
}
}
@ -90,6 +115,9 @@ public class ServerCompilerScheduler implements Scheduler {
private Collection<Node> nodes;
private Map<InputNode, Node> inputNodeToNode;
private Vector<InputBlock> blocks;
// CFG successors of each CFG node, excluding self edges.
Map<Node, List<Node>> controlSuccs = new HashMap<>();
// Nodes reachable in backward traversal from root.
private Map<InputBlock, InputBlock> dominatorMap;
private static final Comparator<InputEdge> edgeComparator = new Comparator<InputEdge>() {
@ -110,8 +138,6 @@ public class ServerCompilerScheduler implements Scheduler {
Stack<Node> stack = new Stack<>();
Set<Node> visited = new HashSet<>();
Map<InputBlock, Set<Node>> terminators = new HashMap<>();
// Pre-compute control successors of each node, excluding self edges.
Map<Node, List<Node>> controlSuccs = new HashMap<>();
for (Node n : nodes) {
if (n.isCFG) {
List<Node> nControlSuccs = new ArrayList<>();
@ -179,7 +205,22 @@ public class ServerCompilerScheduler implements Scheduler {
// (dead) 'Safepoint' nodes.
s.block = block;
blockTerminators.add(s);
for (Node ps : controlSuccs.get(s)) {
List<Node> projSuccs = controlSuccs.get(s);
if (projSuccs.size() > 1) {
s.addWarning(Node.WARNING_BLOCK_PROJECTION_WITH_MULTIPLE_SUCCS);
}
// If s has only one CFG successor ss (regular
// case), there is a node pinned to s, and ss has
// multiple CFG predecessors, insert a block between
// s and ss. This is done by adding a dummy CFG node
// that has no correspondence in the input graph.
if (projSuccs.size() == 1 &&
s.succs.stream().anyMatch(ss -> pinnedNode(ss) == s) &&
projSuccs.get(0).preds.stream().filter(ssp -> ssp.isCFG).count() > 1) {
stack.push(insertDummyCFGNode(s, projSuccs.get(0)));
continue;
}
for (Node ps : projSuccs) {
stack.push(ps);
}
} else {
@ -230,6 +271,24 @@ public class ServerCompilerScheduler implements Scheduler {
}
}
// Create a dummy CFG node (without correspondence in the input graph) and
// insert it between p and s.
private Node insertDummyCFGNode(Node p, Node s) {
// Create in-between node.
Node n = new Node();
n.preds.add(p);
n.succs.add(s);
controlSuccs.put(n, Arrays.asList(s));
n.isCFG = true;
// Update predecessor node p.
p.succs.remove(s);
p.succs.add(n);
controlSuccs.put(p, Arrays.asList(n));
// Update successor node s.
Collections.replaceAll(s.preds, p, n);
return n;
}
private String getBlockName(InputNode n) {
return n.getProperties().get("block");
}
@ -265,6 +324,7 @@ public class ServerCompilerScheduler implements Scheduler {
markCFGNodes();
connectOrphansAndWidows();
buildBlocks();
schedulePinned();
buildDominators();
scheduleLatest();
@ -282,6 +342,8 @@ public class ServerCompilerScheduler implements Scheduler {
}
scheduleLocal();
check();
reportWarnings();
return blocks;
}
@ -379,28 +441,42 @@ public class ServerCompilerScheduler implements Scheduler {
return schedule;
}
private void scheduleLatest() {
Node root = findRoot();
if(root == null) {
assert false : "No root found!";
return;
}
// Mark all nodes reachable in backward traversal from root
Set<Node> reachable = new HashSet<>();
reachable.add(root);
Stack<Node> stack = new Stack<>();
stack.push(root);
while (!stack.isEmpty()) {
Node cur = stack.pop();
for (Node n : cur.preds) {
if (!reachable.contains(n)) {
reachable.add(n);
stack.push(n);
// Return latest block that dominates all successors of n, or null if any
// successor is not yet scheduled.
private InputBlock commonDominatorOfSuccessors(Node n, Set<Node> reachable) {
InputBlock block = null;
for (Node s : n.succs) {
if (!reachable.contains(s)) {
// Unreachable successors should not affect the schedule.
continue;
}
if (s.block == null) {
// Successor is not yet scheduled, wait for it.
return null;
} else if (isPhi(s)) {
// Move inputs above their source blocks.
boolean foundSourceBlock = false;
for (InputBlock srcBlock : sourceBlocks(n, s)) {
foundSourceBlock = true;
block = getCommonDominator(block, srcBlock);
}
if (!foundSourceBlock) {
// Can happen due to inconsistent phi-region pairs.
block = s.block;
n.addWarning(Node.WARNING_PHI_INPUT_WITHOUT_REGION);
}
} else {
// Common case, update current block to also dominate s.
block = getCommonDominator(block, s.block);
}
}
return block;
}
private void scheduleLatest() {
// Mark all nodes reachable in backward traversal from root
Set<Node> reachable = reachableNodes();
Set<Node> unscheduled = new HashSet<>();
for (Node n : this.nodes) {
if (n.block == null && reachable.contains(n)) {
@ -413,28 +489,7 @@ public class ServerCompilerScheduler implements Scheduler {
Set<Node> newUnscheduled = new HashSet<>();
for (Node n : unscheduled) {
InputBlock block = null;
if (this.isPhi(n) && n.preds.get(0) != null) {
// Phi nodes in same block as region nodes
block = n.preds.get(0).block;
} else {
for (Node s : n.succs) {
if (reachable.contains(s)) {
if (s.block == null) {
block = null;
break;
} else {
if (block == null) {
block = s.block;
} else {
block = getCommonDominator(block, s.block);
}
}
}
}
}
InputBlock block = commonDominatorOfSuccessors(n, reachable);
if (block != null) {
n.block = block;
block.addNode(n.inputNode.getId());
@ -464,6 +519,40 @@ public class ServerCompilerScheduler implements Scheduler {
}
// Recompute the input array of the given node, including empty slots.
private Node[] inputArray(Node n) {
Node[] inputs = new Node[Collections.max(n.predIndices) + 1];
for (int i = 0; i < n.preds.size(); i++) {
inputs[n.predIndices.get(i)] = n.preds.get(i);
}
return inputs;
}
// Find the blocks from which node 'in' flows into 'phi'.
private Set<InputBlock> sourceBlocks(Node in, Node phi) {
Node reg = phi.preds.get(0);
assert (reg != null);
// Reconstruct the positional input arrays of phi-region pairs.
Node[] phiInputs = inputArray(phi);
Node[] regInputs = inputArray(reg);
Set<InputBlock> srcBlocks = new HashSet<>();
for (int i = 0; i < Math.min(phiInputs.length, regInputs.length); i++) {
if (phiInputs[i] == in) {
if (regInputs[i] != null) {
if (regInputs[i].isCFG) {
srcBlocks.add(regInputs[i].block);
} else {
reg.addWarning(Node.WARNING_REGION_WITHOUT_CONTROL_INPUT);
}
} else {
phi.addWarning(Node.WARNING_PHI_WITH_REGIONLESS_INPUTS);
}
}
}
return srcBlocks;
}
private void markWithBlock(Node n, InputBlock b, Set<Node> reachable) {
assert !reachable.contains(n);
Stack<Node> stack = new Stack<>();
@ -498,6 +587,12 @@ public class ServerCompilerScheduler implements Scheduler {
if (ba == bb) {
return ba;
}
if (ba == null) {
return bb;
}
if (bb == null) {
return ba;
}
Set<InputBlock> visited = new HashSet<>();
while (ba != null) {
visited.add(ba);
@ -515,6 +610,45 @@ public class ServerCompilerScheduler implements Scheduler {
return null;
}
// Schedule nodes pinned to region-like nodes in the same block. Schedule
// nodes pinned to block projections s in their successor block ss.
// buildBlocks() guarantees that s is the only predecessor of ss.
public void schedulePinned() {
Set<Node> reachable = reachableNodes();
for (Node n : nodes) {
if (!reachable.contains(n) ||
n.block != null) {
continue;
}
Node ctrlIn = pinnedNode(n);
if (ctrlIn == null) {
continue;
}
InputBlock block = ctrlIn.block;
if (ctrlIn.isBlockProjection) {
// Block projections should not have successors in their block:
// if n is pinned to a block projection, push it downwards.
if (controlSuccs.get(ctrlIn).size() == 1) {
block = controlSuccs.get(ctrlIn).get(0).block;
}
}
n.block = block;
block.addNode(n.inputNode.getId());
}
}
// Return the control node to which 'n' is pinned, or null if none.
public Node pinnedNode(Node n) {
if (n.preds.isEmpty()) {
return null;
}
Node ctrlIn = n.preds.get(0);
if (!isControl(ctrlIn)) {
return null;
}
return ctrlIn;
}
public void buildDominators() {
dominatorMap = new HashMap<>(graph.getBlocks().size());
if (blocks.size() == 0) {
@ -560,6 +694,10 @@ public class ServerCompilerScheduler implements Scheduler {
return hasName(n, "Parm");
}
private static boolean isRegion(Node n) {
return hasName(n, "Region");
}
private static boolean hasName(Node n, String name) {
String nodeName = n.inputNode.getProperties().get("name");
if (nodeName == null) {
@ -572,6 +710,18 @@ public class ServerCompilerScheduler implements Scheduler {
return n.inputNode.getProperties().get("category").equals("control");
}
// Whether b1 dominates b2. Used only for checking the schedule.
private boolean dominates(InputBlock b1, InputBlock b2) {
InputBlock bi = b2;
do {
if (bi.equals(b1)) {
return true;
}
bi = dominatorMap.get(bi);
} while (bi != null);
return false;
}
private Node findRoot() {
Node minNode = null;
Node alternativeRoot = null;
@ -597,6 +747,28 @@ public class ServerCompilerScheduler implements Scheduler {
}
}
// Find all nodes reachable in backward traversal from root.
private Set<Node> reachableNodes() {
Node root = findRoot();
if (root == null) {
assert false : "No root found!";
}
Set<Node> reachable = new HashSet<>();
reachable.add(root);
Stack<Node> stack = new Stack<>();
stack.push(root);
while (!stack.isEmpty()) {
Node cur = stack.pop();
for (Node n : cur.preds) {
if (!reachable.contains(n)) {
reachable.add(n);
stack.push(n);
}
}
}
return reachable;
}
public boolean hasCategoryInformation() {
for (InputNode n : graph.getNodes()) {
if (n.getProperties().get("category") == null) {
@ -643,6 +815,7 @@ public class ServerCompilerScheduler implements Scheduler {
Node fromNode = inputNodeToNode.get(fromInputNode);
fromNode.succs.add(toNode);
toNode.preds.add(fromNode);
toNode.predIndices.add(e.getToIndex());
}
}
}
@ -707,4 +880,71 @@ public class ServerCompilerScheduler implements Scheduler {
}
}
}
// Check invariants in the input graph and in the output schedule, and add
// warnings to nodes where the invariants do not hold.
public void check() {
Set<Node> reachable = reachableNodes();
for (Node n : nodes) {
// Check that region nodes are well-formed.
if (isRegion(n) && !n.isBlockStart) {
n.addWarning(Node.WARNING_NOT_MARKED_WITH_BLOCK_START);
}
if (!isPhi(n)) {
continue;
}
if (!reachable.contains(n)) { // Dead phi.
continue;
}
// Check that phi nodes and their inputs are well-formed.
for (int i = 1; i < n.preds.size(); i++) {
Node in = n.preds.get(i);
if (in.isCFG) {
// This can happen for nodes misclassified as CFG, for
// example x64's 'rep_stos'.
in.addWarning(Node.WARNING_CFG_AND_INPUT_TO_PHI);
continue;
}
for (InputBlock b : sourceBlocks(in, n)) {
if (!dominates(graph.getBlock(in.inputNode), b)) {
in.addWarning(Node.WARNING_PHI_NON_DOMINATING_INPUTS);
}
}
}
}
}
// Report potential and actual innacuracies in the schedule approximation.
// IGV tries to warn, rather than crashing, for robustness: an inaccuracy in
// the schedule approximation or an inconsistency in the input graph should
// not disable all IGV functionality, and debugging inconsistent graphs is a
// key use case of IGV. Warns are reported visually for each node (if the
// corresponding filter is active) and textually in the IGV log.
public void reportWarnings() {
Map<String, Set<Node>> nodesPerWarning = new HashMap<>();
for (Node n : nodes) {
if (n.warnings == null || n.warnings.isEmpty()) {
continue;
}
for (String warning : n.warnings) {
if (!nodesPerWarning.containsKey(warning)) {
nodesPerWarning.put(warning, new HashSet<Node>());
}
nodesPerWarning.get(warning).add(n);
}
// Attach warnings to each node as a property to be shown in the
// graph views.
String nodeWarnings = String.join("; ", n.warnings);
n.inputNode.getProperties().setProperty("warnings", nodeWarnings);
}
// Report warnings textually.
for (Map.Entry<String, Set<Node>> entry : nodesPerWarning.entrySet()) {
String warning = entry.getKey();
Set<Node> nodes = entry.getValue();
String nodeList = nodes.toString().replace("[", "(").replace("]", ")");
String message = warning + " " + nodeList;
ErrorManager.getDefault().log(ErrorManager.WARNING, message);
}
}
}

View File

@ -9,9 +9,13 @@
<attr name="enabled" boolvalue="false"/>
<attr name="after" stringvalue="Color by category"/>
</file>
<file name="Show node warnings" url="filters/showWarnings.filter">
<attr name="enabled" boolvalue="true"/>
<attr name="after" stringvalue="Color by execution frequency"/>
</file>
<file name="Simplify graph" url="filters/structural.filter">
<attr name="enabled" boolvalue="false"/>
<attr name="after" stringvalue="Color by execution frequency"/>
<attr name="after" stringvalue="Show node warnings"/>
</file>
<file name="Hide data subgraph" url="filters/hideData.filter">
<attr name="enabled" boolvalue="false"/>

View File

@ -48,12 +48,14 @@ import org.netbeans.api.visual.action.WidgetAction;
import org.netbeans.api.visual.layout.LayoutFactory;
import org.netbeans.api.visual.layout.LayoutFactory.SerialAlignment;
import org.netbeans.api.visual.model.ObjectState;
import org.netbeans.api.visual.widget.ImageWidget;
import org.netbeans.api.visual.widget.LabelWidget;
import org.netbeans.api.visual.widget.Widget;
import org.openide.nodes.AbstractNode;
import org.openide.nodes.Children;
import org.openide.nodes.Node;
import org.openide.nodes.Sheet;
import org.openide.util.ImageUtilities;
import org.openide.util.Lookup;
/**
@ -73,6 +75,7 @@ public class FigureWidget extends Widget implements Properties.Provider, PopupMe
private boolean boundary;
private final Node node;
private Widget dummyTop;
private static final Image warningSign = ImageUtilities.loadImage("com/sun/hotspot/igv/view/images/warning.png");
public void setBoundary(boolean b) {
boundary = b;
@ -123,13 +126,22 @@ public class FigureWidget extends Widget implements Properties.Provider, PopupMe
dummyTop.setMinimumSize(new Dimension(Figure.INSET / 2, 1 + extraTopHeight));
middleWidget.addChild(dummyTop);
// This widget includes the node text and possibly a warning sign to the right.
Widget nodeInfoWidget = new Widget(scene);
nodeInfoWidget.setLayout(LayoutFactory.createAbsoluteLayout());
middleWidget.addChild(nodeInfoWidget);
Widget textWidget = new Widget(scene);
textWidget.setLayout(LayoutFactory.createVerticalFlowLayout(textAlign, 0));
nodeInfoWidget.addChild(textWidget);
String[] strings = figure.getLines();
labelWidgets = new ArrayList<>(strings.length);
for (String displayString : strings) {
LabelWidget lw = new LabelWidget(scene);
labelWidgets.add(lw);
middleWidget.addChild(lw);
textWidget.addChild(lw);
lw.setLabel(displayString);
lw.setFont(figure.getDiagram().getFont());
lw.setForeground(getTextColor());
@ -138,6 +150,14 @@ public class FigureWidget extends Widget implements Properties.Provider, PopupMe
lw.setBorder(BorderFactory.createEmptyBorder());
}
if (getFigure().getWarning() != null) {
ImageWidget warningWidget = new ImageWidget(scene, warningSign);
Point warningLocation = new Point(getFigure().getWidth() - Figure.WARNING_WIDTH - Figure.INSET / 2, 0);
warningWidget.setPreferredLocation(warningLocation);
warningWidget.setToolTipText(getFigure().getWarning());
nodeInfoWidget.addChild(warningWidget);
}
Widget dummyBottom = new Widget(scene);
int extraBottomHeight =
getFigure().getDiagram().isCFG() && getFigure().hasNamedOutputSlot() ?

Binary file not shown.

After

Width:  |  Height:  |  Size: 1.8 KiB