Make ICs no longer be operands

Clone WrappedIRClosure (mostly) now Simplify creation logic in IRClosure and IRScope with a single prepareForInterpretation.
jruby · Oct 20, 2014 · 1134fe3 · 1134fe3
1 parent e71749f
commit 1134fe3
Showing 7 changed files with 18 additions and 73 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRClosure.java b/core/src/main/java/org/jruby/ir/IRClosure.java
@@ -108,21 +108,9 @@ public IRClosure(IRManager manager, IRScope lexicalParent, int lineNumber, Stati
         this.nestingDepth++;
     }
 
-    public InterpreterContext prepareInterpreterContext(Operand self) {
-        if (interpreterContext != null) return interpreterContext; // Already prepared
-
-        initScope(false);
-
-        interpreterContext = new ClosureInterpreterContext(this, prepareInstructions(), self, getBlockBody());
-
-        return interpreterContext;
-    }
-
     @Override
-    public synchronized InterpreterContext prepareForInterpretation() {
-        // This should have already been prepared during preparation of parent scopes.
-        // If this is null, it would be a bug and let users throw a NPE.
-        return interpreterContext;
+    public InterpreterContext allocateInterpreterContext(Instr[] instructionList) {
+        return new ClosureInterpreterContext(this, instructionList);
     }
 
     public void setBeginEndBlock() {

diff --git a/core/src/main/java/org/jruby/ir/IRScope.java b/core/src/main/java/org/jruby/ir/IRScope.java
@@ -611,6 +611,11 @@ protected void initScope(boolean jitMode) {
         }
     }
 
+    /** Make version specific to scope which needs it (e.g. Closure vs non-closure). */
+    public InterpreterContext allocateInterpreterContext(Instr[] instructionList) {
+        return new InterpreterContext(this, instructionList);
+    }
+
     /** Run any necessary passes to get the IR ready for interpretation */
     public synchronized InterpreterContext prepareForInterpretation() {
         if (interpreterContext != null) return interpreterContext; // Already prepared
@@ -619,7 +624,7 @@ public synchronized InterpreterContext prepareForInterpretation() {
 
         // System.out.println("-- passes run for: " + this + " = " + java.util.Arrays.toString(executedPasses.toArray()));
 
-        interpreterContext = new InterpreterContext(this, prepareInstructions());
+        interpreterContext = allocateInterpreterContext(prepareInstructions());
 
         return interpreterContext;
     }

diff --git a/core/src/main/java/org/jruby/ir/instructions/BuildLambdaInstr.java b/core/src/main/java/org/jruby/ir/instructions/BuildLambdaInstr.java
@@ -84,7 +84,7 @@ public Object interpret(ThreadContext context, StaticScope currScope, DynamicSco
         // SSS FIXME: Copied this from ast/LambdaNode ... Is this required here as well?
         //
         // JRUBY-5686: do this before executing so first time sets cref module
-        ((ClosureInterpreterContext)getLambdaBody()).getStaticScope().determineModule();
+        ((WrappedIRClosure) getLambdaBody()).getClosure().getStaticScope().determineModule();
 
         // CON: This must not be happening, because nil would never cast to Block
 //        IRClosure body = getLambdaBody().getClosure();

diff --git a/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java b/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java
@@ -81,7 +81,7 @@ public static IRubyObject interpretCommonEval(Ruby runtime, String file, int lin
         // ClosureInterpreterContext never retrieved as an operand in this context.
         // So, self operand is not required here.
         // Passing null to force early crasher if ever used differently.
-        evalScript.prepareInterpreterContext(null);
+        evalScript.prepareForInterpretation();
         ThreadContext context = runtime.getCurrentContext();
 
         IRubyObject rv = null;
@@ -122,7 +122,7 @@ public static void runBeginEndBlocks(List<IRClosure> beBlocks, ThreadContext con
 
         for (IRClosure b: beBlocks) {
             // SSS FIXME: Should I piggyback on WrappedIRClosure.retrieve or just copy that code here?
-            b.prepareInterpreterContext(b.getSelf());
+            b.prepareForInterpretation();
             Block blk = (Block)(new WrappedIRClosure(b.getSelf(), b)).retrieve(context, self, currScope, context.getCurrentScope(), temp);
             blk.yield(context, null);
         }

diff --git a/core/src/main/java/org/jruby/ir/operands/ClosureInterpreterContext.java b/core/src/main/java/org/jruby/ir/operands/ClosureInterpreterContext.java
@@ -2,27 +2,15 @@
 
 import org.jruby.ir.IRClosure;
 import org.jruby.ir.instructions.Instr;
-import org.jruby.parser.StaticScope;
-import org.jruby.runtime.Binding;
-import org.jruby.runtime.Block;
-import org.jruby.runtime.BlockBody;
 import org.jruby.runtime.DynamicScope;
 import org.jruby.runtime.ThreadContext;
-import org.jruby.runtime.builtin.IRubyObject;
 
 /**
  * Interpreter knowledge needed to interpret a closure.
  */
 public class ClosureInterpreterContext extends InterpreterContext {
-    private Operand self;
-    private BlockBody body;
-
-    public ClosureInterpreterContext(IRClosure scope, Instr[] instructions,
-                                     Operand self, BlockBody body) {
+    public ClosureInterpreterContext(IRClosure scope, Instr[] instructions) {
         super(scope, instructions);
-
-        this.self = self;
-        this.body = body;
     }
 
     /**
@@ -33,17 +21,4 @@ public ClosureInterpreterContext(IRClosure scope, Instr[] instructions,
     public DynamicScope newDynamicScope(ThreadContext context) {
         throw new RuntimeException("We do not push bindings for closures");
     }
-
-    @Override
-    public Object retrieve(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp) {
-        getStaticScope().determineModule();
-
-        // In non-inlining scenarios, this.self will always be %self.
-        // However, in inlined scenarios, this.self will be the self in the original scope where the closure
-        // was present before inlining.
-        IRubyObject selfVal = (this.self instanceof Self) ? self : (IRubyObject) this.self.retrieve(context, self, currScope, currDynScope, temp);
-        Binding binding = context.currentBinding(selfVal, currDynScope);
-
-        return new Block(body, binding);
-    }
 }
diff --git a/core/src/main/java/org/jruby/ir/operands/InterpreterContext.java b/core/src/main/java/org/jruby/ir/operands/InterpreterContext.java
@@ -1,18 +1,15 @@
 package org.jruby.ir.operands;
 
-import java.util.List;
 import org.jruby.ir.IRFlags;
 import org.jruby.ir.IRMetaClassBody;
 import org.jruby.ir.IRScope;
 import org.jruby.ir.instructions.Instr;
 import org.jruby.ir.representations.CFG;
-import org.jruby.ir.transformations.inlining.CloneInfo;
 import org.jruby.parser.StaticScope;
 import org.jruby.runtime.DynamicScope;
 import org.jruby.runtime.ThreadContext;
-import org.jruby.runtime.builtin.IRubyObject;
 
-public class InterpreterContext extends Operand {
+public class InterpreterContext {
     private final int temporaryVariablecount;
     private final int temporaryBooleanVariablecount;
     private final int temporaryFixnumVariablecount;
@@ -38,8 +35,6 @@ public class InterpreterContext extends Operand {
     private CFG cfg = null;
 
     public InterpreterContext(IRScope scope, Instr[] instructions) {
-        super(null);
-
         //FIXME: Remove once we conditionally plug in CFG on debug-only
         this.cfg = scope.getCFG();
 
@@ -60,14 +55,6 @@ public InterpreterContext(IRScope scope, Instr[] instructions) {
         this.receivesKeywordArguments = scope.getFlags().contains(IRFlags.RECEIVES_KEYWORD_ARGS);
     }
 
-    @Override
-    public void addUsedVariables(List<Variable> l) {}
-
-    @Override
-    public Operand cloneForInlining(CloneInfo info) {
-        throw new IllegalStateException("Should not clone interp context");
-    }
-
     public String getFileName() {
         return fileName;
     }
@@ -127,11 +114,6 @@ public boolean receivesKeywordArguments() {
         return receivesKeywordArguments;
     }
 
-    @Override
-    public Object retrieve(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp) {
-        return super.retrieve(context, self, currScope, currDynScope, temp);
-    }
-
     @Override
     public String toString() {
         StringBuilder buf = new StringBuilder();

diff --git a/core/src/main/java/org/jruby/ir/operands/WrappedIRClosure.java b/core/src/main/java/org/jruby/ir/operands/WrappedIRClosure.java
@@ -53,17 +53,12 @@ public Operand getSimplifiedOperand(Map<Operand, Operand> valueMap, boolean forc
 
     @Override
     public Operand cloneForInlining(CloneInfo info) {
-        if (info instanceof SimpleCloneInfo) {
-            SimpleCloneInfo simpleCloneInfo = (SimpleCloneInfo) info;
+        // Making interp instrs so that if JIT hits IRClosure we will not concurrently modify the same IRScope.
+        if (info instanceof SimpleCloneInfo && !((SimpleCloneInfo) info).isEnsureBlockCloneMode()) {
+            closure.prepareForInterpretation(); // makes context as a side-effect
 
-            // For IRBuilding we are pre-interpretation and do want a traditional simple clone
-            if (simpleCloneInfo.isEnsureBlockCloneMode()) {
-                return new WrappedIRClosure(info.getRenamedVariable(self), closure.cloneForInlining(info));
-            }
-
-            // We are saving instructions so that if JIT hits IRClosure, it will not concurrently
-            // modify the same object.
-            return closure.prepareInterpreterContext(self);
+            // FIXME: It really bothers me we do not clone closure here but cloning like main clone case loses interpContext + other things.
+            return new WrappedIRClosure(info.getRenamedVariable(self), closure);
         }
 
         return new WrappedIRClosure(info.getRenamedVariable(self), closure.cloneForInlining(info));