Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@davleopo
davleopo committed Jan 26, 2023
1 parent 56840f6 commit 7732fa2
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 12 deletions.
55 changes: 47 additions & 8 deletions ...piler.loop.phases/src/org/graalvm/compiler/loop/phases/SpeculativeGuardMovementPhase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.graalvm.compiler.nodes.GraphState.StageFlag;
import org.graalvm.compiler.nodes.GuardNode;
import org.graalvm.compiler.nodes.GuardedValueNode;
import org.graalvm.compiler.nodes.LogicConstantNode;
import org.graalvm.compiler.nodes.LogicNode;
import org.graalvm.compiler.nodes.LoopBeginNode;
import org.graalvm.compiler.nodes.NodeView;
Expand Down Expand Up @@ -448,6 +449,42 @@ private static class OptimizedCompareTests {
this.initTest = initTest;
this.extremumTest = extremumTest;
}

private static boolean isLogicConstant(ValueNode v) {
return v instanceof LogicConstantNode;
}

private boolean constantInitTestOrValue(boolean value) {
if (initTestIsConstant()) {
return initTestAsConstant();
}
return value;
}

private boolean constantExtremumTestOrValue(boolean value) {
if (extremumTestIsConstant()) {
return extremumTestAsConstant();
}
return value;
}

private boolean initTestAsConstant() {
assert isLogicConstant(initTest);
return ((LogicConstantNode) initTest).getValue();
}

private boolean extremumTestAsConstant() {
assert isLogicConstant(extremumTest);
return ((LogicConstantNode) extremumTest).getValue();
}

private boolean initTestIsConstant() {
return isLogicConstant(initTest);
}

private boolean extremumTestIsConstant() {
return isLogicConstant(extremumTest);
}
}

private LogicNode createLoopEnterCheck(CountedLoopInfo countedLoop, LogicNode newCompare) {
Expand Down Expand Up @@ -632,23 +669,25 @@ private OptimizedCompareTests shouldOptimizeCompare(CompareNode compare, Inducti
InductionVariable countedLoopInitModifiedIV = iv.getLoop().counted().getBodyIV();
boolean initIsParentIV = false;
boolean initIsParentPhi = false;
ValueNode currentRootInit = currentIv.getRootIV().initNode();
while (currentLoop.parent() != null &&
// init is outer IV node
((initIsParentIV = currentLoop.parent().getInductionVariables().containsKey(currentIv.getRootIV().initNode())) ||
((initIsParentIV = currentLoop.parent().getInductionVariables().containsKey(currentRootInit)) ||
// init is outer phi but not IV
(initIsParentPhi = currentLoop.parent().loopBegin().isPhiAtMerge(currentIv.getRootIV().initNode())))) {
(initIsParentPhi = currentLoop.parent().loopBegin().isPhiAtMerge(currentRootInit)))) {
if (initIsParentIV) {
InductionVariable parentIv = currentLoop.parent().getInductionVariables().get(currentIv.getRootIV().initNode());
InductionVariable parentIv = currentLoop.parent().getInductionVariables().get(currentRootInit);
currentIv = currentIv.duplicateWithNewInit(parentIv.entryTripValue());
} else if (initIsParentPhi) {
currentIv = currentIv.duplicateWithNewInit(((PhiNode) currentIv.getRootIV().initNode()).valueAt(0));
currentIv = currentIv.duplicateWithNewInit(((PhiNode) currentRootInit).valueAt(0));
} else {
throw GraalError.shouldNotReachHere("Must have never entered loop");
}
if (currentLoop.parent().getInductionVariables().containsKey(countedLoopInitModifiedIV.getRootIV().initNode())) {
InductionVariable parentIVBodyRef = currentLoop.parent().getInductionVariables().get(countedLoopInitModifiedIV.getRootIV().initNode());
countedLoopInitModifiedIV = countedLoopInitModifiedIV.duplicateWithNewInit(parentIVBodyRef.entryTripValue());
}
currentRootInit = currentIv.getRootIV().initNode();
currentLoop = currentLoop.parent();
}
if (currentLoop != iv.getLoop()) {
Expand All @@ -662,7 +701,7 @@ private OptimizedCompareTests shouldOptimizeCompare(CompareNode compare, Inducti
OptimizedCompareTests testStripMinedIV = computeNewCompareGuards(compare, duplicateOriginalLoopIV, bound, mirrored, iv.getLoop().counted().getOverFlowGuard(),
outerLoopInitBasedMaxTripCount);
if (optimizedCompareUnconditionalDeopt(guard, testStripMinedIV)) {
debug.log("shouldOptimizeCompare(%s): guard would immediately deopt in strip mined inner loop", compare);
debug.log("shouldOptimizeCompare(%s): guard would immediately deopt in loop", compare);
return null;
}
}
Expand All @@ -681,10 +720,10 @@ private OptimizedCompareTests shouldOptimizeCompare(CompareNode compare, Inducti
* fail or not.
*/
private static boolean optimizedCompareUnconditionalDeopt(GuardNode guard, OptimizedCompareTests tests) {
if (tests.extremumTest.isJavaConstant() || tests.initTest.isJavaConstant()) {
if (tests.extremumTestIsConstant() || tests.initTestIsConstant()) {
// true is the neutral value of &&
final boolean t1 = tests.extremumTest.isConstant() ? tests.extremumTest.asJavaConstant().asBoolean() : true;
final boolean t2 = tests.initTest.isConstant() ? tests.initTest.asJavaConstant().asBoolean() : true;
final boolean t1 = tests.constantExtremumTestOrValue(true);
final boolean t2 = tests.constantInitTestOrValue(true);
final boolean result = t1 && t2;
return result == guard.deoptsOnTrue();
}
Expand Down
6 changes: 2 additions & 4 deletions compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/ValueNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,12 @@ public final JavaKind getStackKind() {
}

/**
* Checks whether this value is a constant.
* Checks whether this value is a constant (i.e. it is of type {@link ConstantNode}.
*
* @return {@code true} if this value is a constant
*/
public final boolean isConstant() {
return this instanceof ConstantNode || this instanceof LogicConstantNode;
return this instanceof ConstantNode;
}

private static final NodePredicate IS_CONSTANT = new NodePredicate() {
Expand Down Expand Up @@ -154,8 +154,6 @@ public final boolean isDefaultConstant() {
public final Constant asConstant() {
if (this instanceof ConstantNode) {
return ((ConstantNode) this).getValue();
} else if (this instanceof LogicConstantNode) {
return JavaConstant.forBoolean(((LogicConstantNode) this).getValue());
} else {
return null;
}
Expand Down
5 changes: 5 additions & 0 deletions ...r/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/loop/CountedLoopInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,12 @@ protected ValueNode maxTripCountNode(boolean assumeLoopEntered, IntegerHelper in
*
* THIS VALUE SHOULD BE TREATED AS UNSIGNED.
*
* Warning: In order to calculate the max trip count it can be necessary to perform a devision
* operation in the generated code before the loop header. If {@code stride is not a power of 2}
* we have to perform an integer division of the range of the induction variable and the stride.
*
* @param assumeLoopEntered if true the check that the loop is entered at all will be omitted.
*
*/
public ValueNode maxTripCountNode(boolean assumeLoopEntered, IntegerHelper integerHelper, ValueNode initNode, ValueNode tripCountLimit) {
StructuredGraph graph = getBodyIV().valueNode().graph();
Expand Down
6 changes: 6 additions & 0 deletions ...compiler.nodes/src/org/graalvm/compiler/nodes/loop/DerivedConvertedInductionVariable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,15 @@ public long constantStride() {

@Override
public ValueNode extremumNode(boolean assumeLoopEntered, Stamp s) {
// base.extremumNode will already perform any necessary conversion operation based on the
// stamp, thus we do not "redo" the same here, the caller decides upon the request result
// stamp bit width
return base.extremumNode(assumeLoopEntered, s);
}

/**
* @see #extremumNode(boolean, Stamp)
*/
@Override
public ValueNode extremumNode(boolean assumeLoopEntered, Stamp s, ValueNode maxTripCount) {
return base.extremumNode(assumeLoopEntered, s, maxTripCount);
Expand Down

0 comments on commit 7732fa2

Please sign in to comment.