Skip to content

Commit

Permalink
[GR-30776] Word operations create division-by-zero deopts in AOT code.
Browse files Browse the repository at this point in the history 
PullRequest: graal/8761
  • Loading branch information
@peter-hofer
peter-hofer committed Apr 16, 2021
2 parents 6a0cc41 + e78955d commit e997cec
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 54 deletions.
18 changes: 0 additions & 18 deletions compiler/src/org.graalvm.compiler.java/src/org/graalvm/compiler/java/BytecodeParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1590,15 +1590,6 @@ protected GuardingNode maybeEmitExplicitStoreCheck(ValueNode array, JavaKind ele
return emitBytecodeExceptionCheck(condition, true, BytecodeExceptionKind.ARRAY_STORE, value);
}

protected GuardingNode maybeEmitExplicitDivisionByZeroCheck(ValueNode y) {
if (!((IntegerStamp) y.stamp(NodeView.DEFAULT)).contains(0) || !needsExplicitDivisionByZeroException(y)) {
return null;
}
ConstantNode zero = ConstantNode.defaultForKind(y.getStackKind(), graph);
LogicNode condition = genUnique(IntegerEqualsNode.create(getConstantReflection(), getMetaAccess(), options, null, y, zero, NodeView.DEFAULT));
return emitBytecodeExceptionCheck(condition, false, BytecodeExceptionKind.DIVISION_BY_ZERO);
}

@Override
public AbstractBeginNode emitBytecodeExceptionCheck(LogicNode condition, boolean passingOnTrue, BytecodeExceptionKind exceptionKind, ValueNode... arguments) {
AbstractBeginNode result = GraphBuilderContext.super.emitBytecodeExceptionCheck(condition, passingOnTrue, exceptionKind, arguments);
Expand Down Expand Up @@ -4870,15 +4861,6 @@ protected boolean needsExplicitStoreCheckException(ValueNode array, ValueNode va
return needsExplicitException();
}

/**
* Returns true if an explicit null check should be emitted for the given object.
*
* @param y The dividend.
*/
protected boolean needsExplicitDivisionByZeroException(ValueNode y) {
return needsExplicitException();
}

@Override
public boolean needsExplicitException() {
BytecodeExceptionMode exceptionMode = graphBuilderConfig.getBytecodeExceptionMode();
Expand Down
12 changes: 12 additions & 0 deletions ...m.compiler.nodes/src/org/graalvm/compiler/nodes/graphbuilderconf/GraphBuilderContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.graalvm.compiler.bytecode.Bytecode;
import org.graalvm.compiler.bytecode.BytecodeProvider;
import org.graalvm.compiler.core.common.type.AbstractPointerStamp;
import org.graalvm.compiler.core.common.type.IntegerStamp;
import org.graalvm.compiler.core.common.type.Stamp;
import org.graalvm.compiler.core.common.type.StampFactory;
import org.graalvm.compiler.core.common.type.StampPair;
Expand All @@ -43,6 +44,7 @@
import org.graalvm.compiler.nodes.BeginNode;
import org.graalvm.compiler.nodes.CallTargetNode;
import org.graalvm.compiler.nodes.CallTargetNode.InvokeKind;
import org.graalvm.compiler.nodes.ConstantNode;
import org.graalvm.compiler.nodes.DynamicPiNode;
import org.graalvm.compiler.nodes.FixedGuardNode;
import org.graalvm.compiler.nodes.FixedWithNextNode;
Expand All @@ -56,6 +58,7 @@
import org.graalvm.compiler.nodes.StateSplit;
import org.graalvm.compiler.nodes.StructuredGraph;
import org.graalvm.compiler.nodes.ValueNode;
import org.graalvm.compiler.nodes.calc.IntegerEqualsNode;
import org.graalvm.compiler.nodes.calc.IsNullNode;
import org.graalvm.compiler.nodes.calc.NarrowNode;
import org.graalvm.compiler.nodes.calc.SignExtendNode;
Expand Down Expand Up @@ -320,6 +323,15 @@ default ValueNode nullCheckedValue(ValueNode value, DeoptimizationAction action)
return value;
}

default GuardingNode maybeEmitExplicitDivisionByZeroCheck(ValueNode divisor) {
if (!needsExplicitException() || !((IntegerStamp) divisor.stamp(NodeView.DEFAULT)).contains(0)) {
return null;
}
ConstantNode zero = add(ConstantNode.defaultForKind(divisor.getStackKind()));
LogicNode condition = add(IntegerEqualsNode.create(getConstantReflection(), getMetaAccess(), getOptions(), null, divisor, zero, NodeView.DEFAULT));
return emitBytecodeExceptionCheck(condition, false, BytecodeExceptionKind.DIVISION_BY_ZERO);
}

default AbstractBeginNode emitBytecodeExceptionCheck(LogicNode condition, boolean passingOnTrue, BytecodeExceptionKind exceptionKind, ValueNode... arguments) {
if (passingOnTrue ? condition.isTautology() : condition.isContradiction()) {
return null;
Expand Down
11 changes: 7 additions & 4 deletions ...nts.aarch64/src/org/graalvm/compiler/replacements/aarch64/AArch64GraphBuilderPlugins.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,16 +255,19 @@ private ArrayCompareToPlugin(JavaKind valueKind, JavaKind otherKind) {

@Override
public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode value, ValueNode other) {
ValueNode valueLength = b.add(new ArrayLengthNode(value));
ValueNode otherLength = b.add(new ArrayLengthNode(other));
ValueNode nonNullValue = b.nullCheckedValue(value);
ValueNode nonNullOther = b.nullCheckedValue(other);

ValueNode valueLength = b.add(new ArrayLengthNode(nonNullValue));
ValueNode otherLength = b.add(new ArrayLengthNode(nonNullOther));
if (swapped) {
/*
* Swapping array arguments because intrinsic expects order to be byte[]/char[] but
* kind arguments stay in original order.
*/
b.addPush(JavaKind.Int, new ArrayCompareToNode(other, value, otherLength, valueLength, valueKind, otherKind));
b.addPush(JavaKind.Int, new ArrayCompareToNode(nonNullOther, nonNullValue, otherLength, valueLength, valueKind, otherKind));
} else {
b.addPush(JavaKind.Int, new ArrayCompareToNode(value, other, valueLength, otherLength, valueKind, otherKind));
b.addPush(JavaKind.Int, new ArrayCompareToNode(nonNullValue, nonNullOther, valueLength, otherLength, valueKind, otherKind));
}
return true;
}
Expand Down
19 changes: 5 additions & 14 deletions ...piler.replacements/src/org/graalvm/compiler/replacements/StandardGraphBuilderPlugins.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ public static void registerInvocationPlugins(MetaAccessProvider metaAccess, Snip
registerStringPlugins(plugins, replacements, snippetReflection, arrayEqualsSubstitution);
registerCharacterPlugins(plugins);
registerShortPlugins(plugins);
registerIntegerLongPlugins(plugins, JavaKind.Int, allowDeoptimization);
registerIntegerLongPlugins(plugins, JavaKind.Long, allowDeoptimization);
registerIntegerLongPlugins(plugins, JavaKind.Int);
registerIntegerLongPlugins(plugins, JavaKind.Long);
registerFloatPlugins(plugins);
registerDoublePlugins(plugins);
if (arrayEqualsSubstitution) {
Expand Down Expand Up @@ -511,7 +511,7 @@ private static void registerUnsafePlugins(Registration r, boolean sunMiscUnsafe,
r.register1("fullFence", Receiver.class, new UnsafeFencePlugin(LOAD_LOAD | STORE_STORE | LOAD_STORE | STORE_LOAD));
}

private static void registerIntegerLongPlugins(InvocationPlugins plugins, JavaKind kind, boolean allowDeoptimization) {
private static void registerIntegerLongPlugins(InvocationPlugins plugins, JavaKind kind) {
Class<?> declaringClass = kind.toBoxedJavaClass();
Class<?> type = kind.toJavaClass();
Registration r = new Registration(plugins, declaringClass);
Expand All @@ -525,30 +525,21 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
r.register2("divideUnsigned", type, type, new InvocationPlugin() {
@Override
public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode dividend, ValueNode divisor) {
GuardingNode zeroCheck = maybeEmitDivisionByZeroCheck(b, divisor, allowDeoptimization);
GuardingNode zeroCheck = b.maybeEmitExplicitDivisionByZeroCheck(divisor);
b.push(kind, b.append(UnsignedDivNode.create(dividend, divisor, zeroCheck, NodeView.DEFAULT)));
return true;
}
});
r.register2("remainderUnsigned", type, type, new InvocationPlugin() {
@Override
public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode dividend, ValueNode divisor) {
GuardingNode zeroCheck = maybeEmitDivisionByZeroCheck(b, divisor, allowDeoptimization);
GuardingNode zeroCheck = b.maybeEmitExplicitDivisionByZeroCheck(divisor);
b.push(kind, b.append(UnsignedRemNode.create(dividend, divisor, zeroCheck, NodeView.DEFAULT)));
return true;
}
});
}

private static GuardingNode maybeEmitDivisionByZeroCheck(GraphBuilderContext b, ValueNode divisor, boolean allowDeoptimization) {
if (allowDeoptimization || !((IntegerStamp) divisor.stamp(NodeView.DEFAULT)).contains(0)) {
return null;
}
ConstantNode zero = b.add(ConstantNode.defaultForKind(divisor.getStackKind()));
LogicNode condition = b.add(IntegerEqualsNode.create(b.getConstantReflection(), b.getMetaAccess(), b.getOptions(), null, divisor, zero, NodeView.DEFAULT));
return b.emitBytecodeExceptionCheck(condition, false, BytecodeExceptionKind.DIVISION_BY_ZERO);
}

private static void registerCharacterPlugins(InvocationPlugins plugins) {
Registration r = new Registration(plugins, Character.class);
r.register1("reverseBytes", char.class, new InvocationPlugin() {
Expand Down
26 changes: 13 additions & 13 deletions compiler/src/org.graalvm.compiler.word/src/org/graalvm/compiler/word/Word.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public static void ensureInitialized() {
*/
public enum Opcode {
NODE_CLASS,
NODE_CLASS_WITH_GUARD,
INTEGER_DIVISION_NODE_CLASS,
COMPARISON,
IS_NULL,
IS_NON_NULL,
Expand Down Expand Up @@ -256,69 +256,69 @@ public Word multiply(Word val) {
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = SignedDivNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = SignedDivNode.class)
public Word signedDivide(SignedWord val) {
return signedDivide((Word) val);
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = SignedDivNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = SignedDivNode.class)
public Word signedDivide(int val) {
return signedDivide(intParam(val));
}

@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = SignedDivNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = SignedDivNode.class)
public Word signedDivide(Word val) {
return box(unbox() / val.unbox());
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = UnsignedDivNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = UnsignedDivNode.class)
public Word unsignedDivide(UnsignedWord val) {
return unsignedDivide((Word) val);
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = UnsignedDivNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = UnsignedDivNode.class)
public Word unsignedDivide(int val) {
return signedDivide(intParam(val));
}

@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = UnsignedDivNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = UnsignedDivNode.class)
public Word unsignedDivide(Word val) {
return box(Long.divideUnsigned(unbox(), val.unbox()));
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = SignedRemNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = SignedRemNode.class)
public Word signedRemainder(SignedWord val) {
return signedRemainder((Word) val);
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = SignedRemNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = SignedRemNode.class)
public Word signedRemainder(int val) {
return signedRemainder(intParam(val));
}

@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = SignedRemNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = SignedRemNode.class)
public Word signedRemainder(Word val) {
return box(unbox() % val.unbox());
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = UnsignedRemNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = UnsignedRemNode.class)
public Word unsignedRemainder(UnsignedWord val) {
return unsignedRemainder((Word) val);
}

@Override
@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = UnsignedRemNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = UnsignedRemNode.class)
public Word unsignedRemainder(int val) {
return signedRemainder(intParam(val));
}

@Operation(opcode = Opcode.NODE_CLASS_WITH_GUARD, node = UnsignedRemNode.class)
@Operation(opcode = Opcode.INTEGER_DIVISION_NODE_CLASS, node = UnsignedRemNode.class)
public Word unsignedRemainder(Word val) {
return box(Long.remainderUnsigned(unbox(), val.unbox()));
}
Expand Down
11 changes: 6 additions & 5 deletions ...iler/src/org.graalvm.compiler.word/src/org/graalvm/compiler/word/WordOperationPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,12 +280,12 @@ protected void processWordOperation(GraphBuilderContext b, ValueNode[] args, Res
}
switch (operation.opcode()) {
case NODE_CLASS:
case NODE_CLASS_WITH_GUARD:
case INTEGER_DIVISION_NODE_CLASS:
assert args.length == 2;
ValueNode left = args[0];
ValueNode right = operation.rightOperandIsInt() ? toUnsigned(b, args[1], JavaKind.Int) : fromSigned(b, args[1]);

b.addPush(returnKind, createBinaryNodeInstance(operation.node(), left, right, operation.opcode() == Opcode.NODE_CLASS_WITH_GUARD));
b.addPush(returnKind, createBinaryNodeInstance(b, operation.node(), left, right, operation.opcode() == Opcode.INTEGER_DIVISION_NODE_CLASS));
break;

case COMPARISON:
Expand Down Expand Up @@ -414,11 +414,12 @@ protected void processWordOperation(GraphBuilderContext b, ValueNode[] args, Res
* method is called for all {@link Word} operations which are annotated with @Operation(node =
* ...) and encapsulates the reflective allocation of the node.
*/
private static ValueNode createBinaryNodeInstance(Class<? extends ValueNode> nodeClass, ValueNode left, ValueNode right, boolean withGuardingNode) {
private static ValueNode createBinaryNodeInstance(GraphBuilderContext b, Class<? extends ValueNode> nodeClass, ValueNode left, ValueNode right, boolean isIntegerDivision) {
try {
Class<?>[] parameterTypes = withGuardingNode ? new Class<?>[]{ValueNode.class, ValueNode.class, GuardingNode.class} : new Class<?>[]{ValueNode.class, ValueNode.class};
GuardingNode zeroCheck = isIntegerDivision ? b.maybeEmitExplicitDivisionByZeroCheck(right) : null;
Class<?>[] parameterTypes = isIntegerDivision ? new Class<?>[]{ValueNode.class, ValueNode.class, GuardingNode.class} : new Class<?>[]{ValueNode.class, ValueNode.class};
Constructor<?> cons = nodeClass.getDeclaredConstructor(parameterTypes);
Object[] initargs = withGuardingNode ? new Object[]{left, right, null} : new Object[]{left, right};
Object[] initargs = isIntegerDivision ? new Object[]{left, right, zeroCheck} : new Object[]{left, right};
return (ValueNode) cons.newInstance(initargs);
} catch (Throwable ex) {
throw new GraalError(ex).addContext(nodeClass.getName());
Expand Down

0 comments on commit e997cec

Please sign in to comment.