Keep track of original scopeDepth in ClosureLocalVariable

* Load/Store local var passes needed to know whether the local var being handled was defined in the same scope in which the var's load/store is being considered. So far, it used a 'definingScope' field for that. However, that requires that all nested closures be cloned whenever a scope is cloned. However, we are no longer doing that => definingScope field in nested scopes went out of sync and screwed around with placement of loads/stores. * This patch gets rid of the scope field and instead uses a boolean to track whether the variable was defined locally or not and updates it during cloneForDepth. Note that it is not sufficient to use a scopeDepth > 0 test because when dynamic scopes are eliminated, the scope depth for all closure local variables get decremented by 1 (=> a non-local variable can have scope depth 0).
jruby · Mar 6, 2015 · d8e4959 · d8e4959
1 parent afdae18
commit d8e4959
Show file tree

Hide file tree

Showing 8 changed files with 33 additions and 16 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRBindingEvalScript.java b/core/src/main/java/org/jruby/ir/IRBindingEvalScript.java
@@ -72,7 +72,7 @@ public LocalVariable getLocalVariable(String name, int scopeDepth) {
     @Override
     public LocalVariable getNewLocalVariable(String name, int depth) {
         assert depth == nearestNonEvalScopeDepth: "Local variable depth in IREvalScript:getNewLocalVariable for " + name + " must be " + nearestNonEvalScopeDepth + ".  Got " + depth;
-        LocalVariable lvar = new ClosureLocalVariable(this, name, 0, nearestNonEvalScope.evalScopeVars.size());
+        LocalVariable lvar = new ClosureLocalVariable(name, 0, nearestNonEvalScope.evalScopeVars.size());
         nearestNonEvalScope.evalScopeVars.put(name, lvar);
         // CON: unsure how to get static scope to reflect this name as in IRClosure and IRMethod
         return lvar;

diff --git a/core/src/main/java/org/jruby/ir/IRClosure.java b/core/src/main/java/org/jruby/ir/IRClosure.java
@@ -182,7 +182,7 @@ protected LocalVariable findExistingLocalVariable(String name, int scopeDepth) {
 
     public LocalVariable getNewLocalVariable(String name, int depth) {
         if (depth == 0 && !(this instanceof IRFor)) {
-            LocalVariable lvar = new ClosureLocalVariable(this, name, 0, getStaticScope().addVariableThisScope(name));
+            LocalVariable lvar = new ClosureLocalVariable(name, 0, getStaticScope().addVariableThisScope(name));
             localVars.put(name, lvar);
             return lvar;
         } else {

diff --git a/core/src/main/java/org/jruby/ir/dataflow/analyses/LoadLocalVarPlacementNode.java b/core/src/main/java/org/jruby/ir/dataflow/analyses/LoadLocalVarPlacementNode.java
@@ -248,7 +248,7 @@ public void addLoads(Map<Operand, Operand> varRenameMap) {
             // System.out.println("\t--> Reqd loads   : " + java.util.Arrays.toString(reqdLoads.toArray()));
             for (LocalVariable v : reqdLoads) {
                 if (scope.usesLocalVariable(v) || scope.definesLocalVariable(v)) {
-                    if (isEvalScript || !(v instanceof ClosureLocalVariable) || (scope != ((ClosureLocalVariable)v).definingScope)) {
+                    if (isEvalScript || !(v instanceof ClosureLocalVariable) || !((ClosureLocalVariable)v).isDefinedLocally()) {
                         it.add(new LoadLocalVarInstr(scope, getLocalVarReplacement(v, scope, varRenameMap), v));
                     }
                 }

diff --git a/core/src/main/java/org/jruby/ir/dataflow/analyses/StoreLocalVarPlacementNode.java b/core/src/main/java/org/jruby/ir/dataflow/analyses/StoreLocalVarPlacementNode.java
@@ -91,7 +91,7 @@ public void applyTransferFunction(Instr i) {
                 // Allocate a new hash-set and modify it to get around ConcurrentModificationException on dirtyVars
                 Set<LocalVariable> newDirtyVars = new HashSet<LocalVariable>(dirtyVars);
                 for (LocalVariable v : dirtyVars) {
-                    if ((v instanceof ClosureLocalVariable) && ((ClosureLocalVariable)v).definingScope != scope) {
+                    if ((v instanceof ClosureLocalVariable) && !((ClosureLocalVariable)v).isDefinedLocally()) {
                         newDirtyVars.remove(v);
                     }
                 }
@@ -217,7 +217,7 @@ public boolean addStores(Map<Operand, Operand> varRenameMap, Set<LocalVariable>
                         // instance from a different depth and that could mislead us. See if there is a way to fix this.
                         // If we introduced 'definingScope' in all local variables, we could simply check for scope match
                         // without the instanceof check here.
-                        if (   (v instanceof ClosureLocalVariable && ((ClosureLocalVariable)v).definingScope != scope)
+                        if (   (v instanceof ClosureLocalVariable && !((ClosureLocalVariable)v).isDefinedLocally())
                             || (!(v instanceof ClosureLocalVariable) && scope.getScopeType().isClosureType()))
                         {
                             addedStores = true;

diff --git a/core/src/main/java/org/jruby/ir/dataflow/analyses/StoreLocalVarPlacementProblem.java b/core/src/main/java/org/jruby/ir/dataflow/analyses/StoreLocalVarPlacementProblem.java
@@ -67,7 +67,7 @@ boolean addClosureExitStoreLocalVars(ListIterator<Instr> instrs, Set<LocalVariab
         boolean addedStores  = false;
         boolean isEvalScript = scope instanceof IREvalScript;
         for (LocalVariable v : dirtyVars) {
-            if (isEvalScript || !(v instanceof ClosureLocalVariable) || (scope != ((ClosureLocalVariable)v).definingScope)) {
+            if (isEvalScript || !(v instanceof ClosureLocalVariable) || !((ClosureLocalVariable)v).isDefinedLocally()) {
                 addedStores = true;
                 instrs.add(new StoreLocalVarInstr(getLocalVarReplacement(v, varRenameMap), scope, v));
             }

diff --git a/core/src/main/java/org/jruby/ir/operands/ClosureLocalVariable.java b/core/src/main/java/org/jruby/ir/operands/ClosureLocalVariable.java
@@ -5,15 +5,21 @@
 import org.jruby.ir.transformations.inlining.SimpleCloneInfo;
 
 /**
- * This represents a variable used in a closure that is
- * local to the closure and is not defined in any ancestor lexical scope
+ * This represents a non-temporary variable used in a closure
+ * and defined in this or a parent closure.
  */
 public class ClosureLocalVariable extends LocalVariable {
-    final public IRClosure definingScope;
+    // Note that we cannot use (scopeDepth > 0) check to detect this.
+    // When a dyn-scope is eliminated for a leaf scope, depths for all
+    // closure local vars are decremented by 1 => a non-local variable
+    // can have scope depth 0.
+    //
+    // Can only transition in one direction (from true to false)
+    private boolean definedLocally;
 
-    public ClosureLocalVariable(IRClosure scope, String name, int scopeDepth, int location) {
+    public ClosureLocalVariable(String name, int scopeDepth, int location) {
         super(name, scopeDepth, location);
-        this.definingScope = scope;
+        this.definedLocally = true;
     }
 
     @Override
@@ -32,13 +38,23 @@ public int compareTo(Object arg0) {
         return a < b ? -1 : (a == b ? 0 : 1);
     }
 
+    public boolean isDefinedLocally() {
+        return definedLocally;
+    }
+
     @Override
     public Variable clone(SimpleCloneInfo ii) {
-        return new ClosureLocalVariable((IRClosure) ii.getScope(), name, scopeDepth, offset);
+        ClosureLocalVariable lv = new ClosureLocalVariable(name, scopeDepth, offset);
+        lv.definedLocally = definedLocally;
+        return lv;
     }
 
     public LocalVariable cloneForDepth(int n) {
-        return new ClosureLocalVariable(definingScope, name, n, offset);
+        ClosureLocalVariable lv = new ClosureLocalVariable(name, n, offset);
+        if (definedLocally && n > 0) {
+            lv.definedLocally = false;
+        }
+        return lv;
     }
 
     @Override
@@ -48,6 +64,6 @@ public void visit(IRVisitor visitor) {
 
     @Override
     public String toString() {
-        return "<" + name + "(" + scopeDepth + ":" + offset + ")>";
+        return "<" + name + "(" + scopeDepth + ":" + offset + ":local=" + definedLocally + ")>";
     }
 }
diff --git a/core/src/main/java/org/jruby/ir/persistence/IRReader.java b/core/src/main/java/org/jruby/ir/persistence/IRReader.java
@@ -92,7 +92,8 @@ private static Map<String, LocalVariable> decodeScopeLocalVariables(IRReaderDeco
             int offset = decoder.decodeInt();
 
             localVariables.put(name, scope instanceof IRClosure ?
-                    new ClosureLocalVariable((IRClosure) scope, name, 0, offset) : new LocalVariable(name, 0, offset));
+                    // SSS FIXME: do we need to read back locallyDefined boolean?
+                    new ClosureLocalVariable(name, 0, offset) : new LocalVariable(name, 0, offset));
         }
 
         return localVariables;

diff --git a/core/src/main/java/org/jruby/ir/persistence/OperandEncoderMap.java b/core/src/main/java/org/jruby/ir/persistence/OperandEncoderMap.java
@@ -39,7 +39,7 @@ public void encode(Operand operand) {
     @Override public void Boolean(Boolean booleanliteral) { encoder.encode(booleanliteral.isTrue()); }
 
     @Override public void ClosureLocalVariable(ClosureLocalVariable variable) {
-        // We can refigure out closure scope it is in.
+        // SSS FIXME: Need to dump definedLocally?
         encoder.encode(variable.getName());
         encoder.encode(variable.getScopeDepth());
     }