From ff75637c454f017b6cba719150c6a63fdf9fc442 Mon Sep 17 00:00:00 2001
From: Adam Welc <adam.welc@oracle.com>
Date: Mon, 23 Mar 2015 09:56:21 -0700
Subject: [PATCH] Backed out changeset 0f217709eb72 to fix performance
regression.
---
.../truffle/r/nodes/RTruffleVisitor.java | 73 ++++++++++---------
.../r/nodes/access/WriteVariableNode.java | 20 ++---
2 files changed, 49 insertions(+), 44 deletions(-)
diff --git a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/RTruffleVisitor.java b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/RTruffleVisitor.java
index 3ec710818f..bf244c9228 100644
--- a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/RTruffleVisitor.java
+++ b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/RTruffleVisitor.java
@@ -355,9 +355,7 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
return new RNode[5];
}
- private static final String varSymbol = "*tmp*";
-
- private static String constructReplacementPrefix(RNode[] seq, RNode rhs, RNode replacementArg, WriteVariableNode.Mode rhsWriteMode) {
+ private static String constructReplacementPrefix(RNode[] seq, RNode rhs, RNode replacementArg, WriteVariableNode.Mode rhsWriteMode, String tmpSymbol) {
//@formatter:off
// store a - need to use temporary, otherwise there is a failure in case multiple calls to
// the replacement form are chained:
@@ -365,8 +363,8 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
//@formatter:on
final String rhsSymbol = new Object().toString();
- WriteVariableNode rhsAssign = WriteVariableNode.create(rhsSymbol.toString(), rhs, false, false, rhsWriteMode);
- WriteVariableNode varAssign = WriteVariableNode.create(varSymbol, replacementArg, false, false, WriteVariableNode.Mode.TEMP);
+ WriteVariableNode rhsAssign = WriteVariableNode.create(rhsSymbol, rhs, false, false, rhsWriteMode);
+ WriteVariableNode varAssign = WriteVariableNode.create(tmpSymbol, replacementArg, false, false, WriteVariableNode.Mode.INVISIBLE);
seq[0] = rhsAssign;
seq[1] = varAssign;
@@ -374,9 +372,9 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
return rhsSymbol;
}
- private static ReplacementNode constructReplacementSuffix(RNode[] seq, RNode assignFromTemp, Object rhsSymbol, SourceSection source) {
+ private static ReplacementNode constructReplacementSuffix(RNode[] seq, RNode assignFromTemp, Object tmpSymbol, Object rhsSymbol, SourceSection source) {
// remove var and rhs, returning rhs' value
- RemoveAndAnswerNode rmVar = RemoveAndAnswerNode.create(varSymbol);
+ RemoveAndAnswerNode rmVar = RemoveAndAnswerNode.create(tmpSymbol);
RemoveAndAnswerNode rmRhs = RemoveAndAnswerNode.create(rhsSymbol);
// assemble
@@ -440,17 +438,18 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
SimpleAccessVariable varAST = (SimpleAccessVariable) a.getVector();
String vSymbol = varAST.getVariable();
+ final String tmpSymbol = new Object().toString();
RNode[] seq = createReplacementSequence();
ReadVariableNode v = isSuper ? ReadVariableNode.createSuperLookup(varAST.getSource(), vSymbol) : ReadVariableNode.create(varAST.getSource(), vSymbol, varAST.shouldCopyValue());
- final Object rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE);
+ final Object rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
String rhsSymbolString = RRuntime.toString(rhsSymbol);
RNode rhsAccess = ReadVariableNode.create(rhsSymbolString, false);
- RNode tmpVarAccess = ReadVariableNode.create(varSymbol, false);
+ RNode tmpVarAccess = ReadVariableNode.create(tmpSymbol, false);
CoerceVector coerceVector = CoerceVectorNodeGen.create(null, null, null);
RNode updateOp = createPositions(a.getArguments(), argLength, a.isSubset(), null, tmpVarAccess, rhsAccess, coerceVector, true);
- RNode assignFromTemp = WriteVariableNode.create(vSymbol, updateOp, false, isSuper, WriteVariableNode.Mode.TEMP);
- return constructReplacementSuffix(seq, assignFromTemp, rhsSymbol, source);
+ RNode assignFromTemp = WriteVariableNode.create(vSymbol, updateOp, false, isSuper);
+ return constructReplacementSuffix(seq, assignFromTemp, tmpSymbol, rhsSymbol, source);
} else if (a.getVector() instanceof AccessVector) {
// assign value to the outermost dimension and then the result (recursively) to
// appropriate position in the lower dimension
@@ -459,15 +458,16 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
AccessVector vecAST = (AccessVector) a.getVector();
SimpleAccessVariable varAST = getVectorVariable(vecAST);
RNode[] seq = new RNode[3];
+ final String tmpSymbol = new Object().toString();
String rhsSymbol;
if (varAST != null) {
String vSymbol = varAST.getVariable();
ReadVariableNode v = isSuper ? ReadVariableNode.createSuperLookup(varAST.getSource(), vSymbol) : ReadVariableNode.create(varAST.getSource(), vSymbol, varAST.shouldCopyValue());
- rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE);
+ rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
} else {
- rhsSymbol = constructReplacementPrefix(seq, rhs, vecAST.getVector().accept(this), WriteVariableNode.Mode.INVISIBLE);
+ rhsSymbol = constructReplacementPrefix(seq, rhs, vecAST.getVector().accept(this), WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
}
RNode rhsAccess = AccessVariable.create(null, rhsSymbol).accept(this);
CoerceVector coerceVector = CoerceVectorNodeGen.create(null, null, null);
@@ -479,12 +479,13 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
RNode[] seq = new RNode[3];
String rhsSymbol;
+ final String tmpSymbol = new Object().toString();
if (varAST != null) {
String vSymbol = varAST.getVariable();
ReadVariableNode v = isSuper ? ReadVariableNode.createSuperLookup(varAST.getSource(), vSymbol) : ReadVariableNode.create(varAST.getSource(), vSymbol, varAST.shouldCopyValue());
- rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE);
+ rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
} else {
- rhsSymbol = constructReplacementPrefix(seq, rhs, accessAST.getLhs().accept(this), WriteVariableNode.Mode.INVISIBLE);
+ rhsSymbol = constructReplacementPrefix(seq, rhs, accessAST.getLhs().accept(this), WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
}
RNode rhsAccess = AccessVariable.create(null, rhsSymbol).accept(this);
@@ -515,10 +516,10 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
return WriteVariableNode.create(n.getSource(), n.getVariable(), expression, false, n.isSuper());
}
- private RCallNode prepareReplacementCall(FunctionCall f, List<ArgNode> args, String rhsSymbol, boolean simpleReplacement) {
+ private RCallNode prepareReplacementCall(FunctionCall f, List<ArgNode> args, String tmpSymbol, String rhsSymbol, boolean simpleReplacement) {
// massage arguments to replacement function call (replace v with tmp, append a)
List<ArgNode> rfArgs = new ArrayList<>();
- rfArgs.add(ArgNode.create(null, null, AccessVariable.create(null, varSymbol, false)));
+ rfArgs.add(ArgNode.create(null, null, AccessVariable.create(null, tmpSymbol, false)));
if (args.size() > 1) {
for (int i = 1; i < args.size(); ++i) {
rfArgs.add(args.get(i));
@@ -555,33 +556,34 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
FunctionCall f = replacement.getReplacementFunctionCall();
List<ArgNode> args = f.getArguments();
ASTNode val = args.get(0).getValue();
+ final String tmpSymbol = new Object().toString();
if (val instanceof SimpleAccessVariable) {
SimpleAccessVariable callArg = (SimpleAccessVariable) val;
String vSymbol = callArg.getVariable();
RNode[] seq = createReplacementSequence();
ReadVariableNode replacementCallArg = createReplacementForVariableUsing(callArg, vSymbol, replacement);
- String rhsSymbol = constructReplacementPrefix(seq, rhs, replacementCallArg, WriteVariableNode.Mode.COPY);
- RNode replacementCall = prepareReplacementCall(f, args, rhsSymbol, true);
- RNode assignFromTemp = WriteVariableNode.create(vSymbol, replacementCall, false, replacement.isSuper(), WriteVariableNode.Mode.TEMP);
- return constructReplacementSuffix(seq, assignFromTemp, rhsSymbol, replacement.getSource());
+ String rhsSymbol = constructReplacementPrefix(seq, rhs, replacementCallArg, WriteVariableNode.Mode.COPY, tmpSymbol);
+ RNode replacementCall = prepareReplacementCall(f, args, tmpSymbol, rhsSymbol, true);
+ RNode assignFromTemp = WriteVariableNode.create(vSymbol, replacementCall, false, replacement.isSuper(), WriteVariableNode.Mode.INVISIBLE);
+ return constructReplacementSuffix(seq, assignFromTemp, tmpSymbol, rhsSymbol, replacement.getSource());
} else if (val instanceof AccessVector) {
AccessVector callArgAst = (AccessVector) val;
RNode replacementArg = callArgAst.accept(this);
RNode[] seq = createReplacementSequence();
- String rhsSymbol = constructReplacementPrefix(seq, rhs, replacementArg, WriteVariableNode.Mode.COPY);
- RNode replacementCall = prepareReplacementCall(f, args, rhsSymbol, false);
+ String rhsSymbol = constructReplacementPrefix(seq, rhs, replacementArg, WriteVariableNode.Mode.COPY, tmpSymbol);
+ RNode replacementCall = prepareReplacementCall(f, args, tmpSymbol, rhsSymbol, false);
// see AssignVariable.writeVector (number of args must match)
callArgAst.getArguments().add(ArgNode.create(rhsAst.getSource(), "value", rhsAst));
RNode assignFromTemp = createVectorUpdate(callArgAst, replacementCall, replacement.isSuper(), replacement.getSource(), false);
- return constructReplacementSuffix(seq, assignFromTemp, rhsSymbol, replacement.getSource());
+ return constructReplacementSuffix(seq, assignFromTemp, tmpSymbol, rhsSymbol, replacement.getSource());
} else {
FieldAccess callArgAst = (FieldAccess) val;
RNode replacementArg = callArgAst.accept(this);
RNode[] seq = createReplacementSequence();
- String rhsSymbol = constructReplacementPrefix(seq, rhs, replacementArg, WriteVariableNode.Mode.COPY);
- RNode replacementCall = prepareReplacementCall(f, args, rhsSymbol, false);
+ String rhsSymbol = constructReplacementPrefix(seq, rhs, replacementArg, WriteVariableNode.Mode.COPY, tmpSymbol);
+ RNode replacementCall = prepareReplacementCall(f, args, tmpSymbol, rhsSymbol, false);
RNode assignFromTemp = createFieldUpdate(callArgAst, replacementCall, replacement.isSuper(), replacement.getSource());
- return constructReplacementSuffix(seq, assignFromTemp, rhsSymbol, replacement.getSource());
+ return constructReplacementSuffix(seq, assignFromTemp, tmpSymbol, rhsSymbol, replacement.getSource());
}
}
@@ -670,25 +672,27 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
RNode[] seq = createReplacementSequence();
ReadVariableNode v = isSuper ? ReadVariableNode.createSuperLookup(varAST.getSource(), vSymbol) : ReadVariableNode.create(varAST.getSource(), vSymbol, varAST.shouldCopyValue());
- final Object rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE);
+ final String tmpSymbol = new Object().toString();
+ final Object rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
String rhsSymbolString = RRuntime.toString(rhsSymbol);
RNode rhsAccess = ReadVariableNode.create(rhsSymbolString, false);
- RNode tmpVarAccess = ReadVariableNode.create(varSymbol, false);
+ RNode tmpVarAccess = ReadVariableNode.create(tmpSymbol, false);
UpdateFieldNode ufn = UpdateFieldNodeGen.create(tmpVarAccess, rhsAccess, a.getFieldName());
- RNode assignFromTemp = WriteVariableNode.create(vSymbol, ufn, false, isSuper, WriteVariableNode.Mode.TEMP);
- return constructReplacementSuffix(seq, assignFromTemp, rhsSymbol, source);
+ RNode assignFromTemp = WriteVariableNode.create(vSymbol, ufn, false, isSuper, WriteVariableNode.Mode.INVISIBLE);
+ return constructReplacementSuffix(seq, assignFromTemp, tmpSymbol, rhsSymbol, source);
} else if (a.getLhs() instanceof AccessVector) {
AccessVector vecAST = (AccessVector) a.getLhs();
SimpleAccessVariable varAST = getVectorVariable(vecAST);
RNode[] seq = new RNode[3];
+ final String tmpSymbol = new Object().toString();
String rhsSymbol;
if (varAST != null) {
String vSymbol = varAST.getVariable();
ReadVariableNode v = isSuper ? ReadVariableNode.createSuperLookup(varAST.getSource(), vSymbol) : ReadVariableNode.create(varAST.getSource(), vSymbol, varAST.shouldCopyValue());
- rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE);
+ rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
} else {
- rhsSymbol = constructReplacementPrefix(seq, rhs, vecAST.getVector().accept(this), WriteVariableNode.Mode.INVISIBLE);
+ rhsSymbol = constructReplacementPrefix(seq, rhs, vecAST.getVector().accept(this), WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
}
RNode rhsAccess = AccessVariable.create(null, rhsSymbol).accept(this);
@@ -705,7 +709,8 @@ public final class RTruffleVisitor extends BasicVisitor<RNode> {
String vSymbol = varAST.getVariable();
RNode[] seq = new RNode[3];
ReadVariableNode v = isSuper ? ReadVariableNode.createSuperLookup(varAST.getSource(), vSymbol) : ReadVariableNode.create(varAST.getSource(), vSymbol, varAST.shouldCopyValue());
- String rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE);
+ final String tmpSymbol = new Object().toString();
+ String rhsSymbol = constructReplacementPrefix(seq, rhs, v, WriteVariableNode.Mode.INVISIBLE, tmpSymbol);
RNode rhsAccess = AccessVariable.create(null, rhsSymbol).accept(this);
UpdateFieldNode ufn = UpdateFieldNodeGen.create(accessAST.accept(this), rhsAccess, a.getFieldName());
return constructRecursiveFieldUpdateSuffix(seq, ufn, accessAST, source, isSuper);
diff --git a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/WriteVariableNode.java b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/WriteVariableNode.java
index 40ec9b1990..61f6bcb6e0 100644
--- a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/WriteVariableNode.java
+++ b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/WriteVariableNode.java
@@ -51,8 +51,7 @@ public abstract class WriteVariableNode extends RNode implements VisibilityContr
REGULAR,
COPY,
- INVISIBLE,
- TEMP
+ INVISIBLE
}
public abstract boolean isArgWrite();
@@ -94,8 +93,14 @@ public abstract class WriteVariableNode extends RNode implements VisibilityContr
protected final Object shareObjectValue(Frame frame, FrameSlot frameSlot, Object value, Mode mode, boolean isSuper) {
Object newValue = value;
if (!isArgWrite()) {
- // for the meaning of INVISIBLE mode see the comment preceding the current method
- if (mode != Mode.INVISIBLE && !isCurrentProfile.profile(isCurrentValue(frame, frameSlot, value))) {
+ // for the meaning of INVISIBLE mode see the comment preceding the current method;
+ // also change state when assigning to the enclosing frame as there must
+ // be a distinction between variables with the same name defined in
+ // different scopes, for example to correctly support:
+ // x<-1:3; f<-function() { x[2]<-10; x[2]<<-100; x[2]<-1000 }; f()
+ // or
+ // x<-c(1); f<-function() { x[[1]]<<-x[[1]] + 1; x }; a<-f(); b<-f(); c(a,b)
+ if ((mode != Mode.INVISIBLE || isSuper) && !isCurrentProfile.profile(isCurrentValue(frame, frameSlot, value))) {
if (isShareableProfile.profile(value instanceof RShareable)) {
RShareable rShareable = (RShareable) value;
if (isTemporaryProfile.profile(rShareable.isTemporary())) {
@@ -115,12 +120,7 @@ public abstract class WriteVariableNode extends RNode implements VisibilityContr
if (mode == Mode.COPY) {
RShareable shareableCopy = rShareable.copy();
newValue = shareableCopy;
- } else if (mode != Mode.TEMP || isSuper) {
- // mark shared when assigning to the enclosing frame as there must
- // be a distinction between variables with the same name defined in
- // different scopes, for example to correctly support:
- // x<-1:3; f<-function() { x[2]<-10; x[2]<<-100; x[2]<-1000 } ; f()
-
+ } else {
rShareable.makeShared();
}
}
--
GitLab