Fix #2409: Splat --> BuildSplatInstr + Splat (for use in call args)

* Splat operand is now a BuildSplatInstr to represent splats seen in ruby code. These can throw exceptions. * Splat operand is now only used in call args and will always extract its args and is guaranteed to have a RubyArray for its operand. These cannot throw exceptions. * This will ungreen master since my jit fixes seem to be incomplete -- Charlie will take over those fixes.
jruby · Jan 5, 2015 · 29371d9 · 29371d9
1 parent 8ced6d2
commit 29371d9
Showing 11 changed files with 127 additions and 27 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -577,11 +577,11 @@ protected Operand buildAttrAssignCallArgs(List<Operand> argsList, Node args, IRS
                 Operand rhs = build(argsPushNode.getSecondNode(), s);
                 Variable res = s.createTemporaryVariable();
                 addInstr(s, new BuildCompoundArrayInstr(res, lhs, rhs, true));
-                argsList.add(new Splat(res, true));
+                argsList.add(new Splat(res));
                 return rhs;
             }
             case SPLATNODE: {     // a[1] = *b
-                Splat rhs = new Splat(build(((SplatNode)args).getValue(), s), true);
+                Splat rhs = new Splat(buildSplat((SplatNode)args, s));
                 argsList.add(rhs);
                 return rhs;
             }
@@ -594,7 +594,7 @@ protected Operand[] buildCallArgs(Node args, IRScope s) {
         switch (args.getNodeType()) {
             case ARGSCATNODE:
             case ARGSPUSHNODE:
-                return new Operand[] { new Splat(build(args, s), true) };
+                return new Operand[] { new Splat(build(args, s)) };
             case ARRAYNODE: {
                 List<Node> children = args.childNodes();
                 int numberOfArgs = children.size();
@@ -606,7 +606,7 @@ protected Operand[] buildCallArgs(Node args, IRScope s) {
                 return builtArgs;
             }
             case SPLATNODE:
-                return new Operand[] { new Splat(build(((SplatNode) args).getValue(), s), true) };
+                return new Operand[] { new Splat(buildSplat((SplatNode)args, s)) };
         }
 
         throw new NotCompilableException("Invalid node for call args: " + args.getClass().getSimpleName() + ":" + args.getPosition());
@@ -3269,9 +3269,9 @@ public Operand buildSelf(IRScope s) {
     }
 
     public Operand buildSplat(SplatNode splatNode, IRScope s) {
-        // SSS: Since splats can only occur in call argument lists, no need to copyAndReturnValue(...)
-        // Verify with Tom / Charlie
-        return new Splat(build(splatNode.getValue(), s));
+        Variable res = s.createTemporaryVariable();
+        addInstr(s, new BuildSplatInstr(res, build(splatNode.getValue(), s)));
+        return res;
     }
 
     public Operand buildStr(StrNode strNode, IRScope s) {

diff --git a/core/src/main/java/org/jruby/ir/IRClosure.java b/core/src/main/java/org/jruby/ir/IRClosure.java
@@ -195,7 +195,7 @@ public void addInstr(Instr i) {
             // FIXME: This lost encoding information when name was converted to string earlier in IRBuilder
             keywordArgs.add(new KeyValuePair<Operand, Operand>(new Symbol(rkai.argName, USASCIIEncoding.INSTANCE), rkai.getResult()));
         } else if (i instanceof ReceiveRestArgInstr) {
-            blockArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult(), true));
+            blockArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult()));
         } else if (i instanceof ReceiveArgBase) {
             blockArgs.add(((ReceiveArgBase) i).getResult());
         }

diff --git a/core/src/main/java/org/jruby/ir/IRMethod.java b/core/src/main/java/org/jruby/ir/IRMethod.java
@@ -97,7 +97,7 @@ public void addInstr(Instr i) {
             // FIXME: This lost encoding information when name was converted to string earlier in IRBuilder
             keywordArgs.add(new KeyValuePair<Operand, Operand>(new Symbol(rkai.argName, USASCIIEncoding.INSTANCE), rkai.getResult()));
         } else if (i instanceof ReceiveRestArgInstr) {
-            callArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult(), true));
+            callArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult()));
         } else if (i instanceof ReceiveArgBase) {
             callArgs.add(((ReceiveArgBase) i).getResult());
         }

diff --git a/core/src/main/java/org/jruby/ir/IRVisitor.java b/core/src/main/java/org/jruby/ir/IRVisitor.java
@@ -45,6 +45,7 @@ private void error(Object object) {
     public void BuildCompoundStringInstr(BuildCompoundStringInstr instr) { error(instr); }
     public void BuildDynRegExpInstr(BuildDynRegExpInstr instr) { error(instr); }
     public void BuildRangeInstr(BuildRangeInstr instr) { error(instr); }
+    public void BuildSplatInstr(BuildSplatInstr instr) { error(instr); }
     public void CallInstr(CallInstr callinstr) { error(callinstr); }
     public void CheckArgsArrayArityInstr(CheckArgsArrayArityInstr checkargsarrayarityinstr) { error(checkargsarrayarityinstr); }
     public void CheckArityInstr(CheckArityInstr checkarityinstr) { error(checkarityinstr); }

diff --git a/core/src/main/java/org/jruby/ir/Operation.java b/core/src/main/java/org/jruby/ir/Operation.java
@@ -143,6 +143,7 @@ public enum Operation {
     BUILD_COMPOUND_STRING(OpFlags.f_can_raise_exception),
     BUILD_DREGEXP(OpFlags.f_can_raise_exception),
     BUILD_RANGE(OpFlags.f_can_raise_exception),
+    BUILD_SPLAT(OpFlags.f_can_raise_exception),
     BACKTICK_STRING(OpFlags.f_can_raise_exception),
     CHECK_ARGS_ARRAY_ARITY(OpFlags.f_can_raise_exception),
     CHECK_ARITY(OpFlags.f_is_book_keeping_op | OpFlags.f_can_raise_exception),

diff --git a/core/src/main/java/org/jruby/ir/instructions/BuildSplatInstr.java b/core/src/main/java/org/jruby/ir/instructions/BuildSplatInstr.java
@@ -0,0 +1,71 @@
+package org.jruby.ir.instructions;
+
+import org.jruby.ir.IRVisitor;
+import org.jruby.ir.Operation;
+import org.jruby.ir.operands.Operand;
+import org.jruby.ir.operands.Variable;
+import org.jruby.ir.runtime.IRRuntimeHelpers;
+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;
+
+import java.util.Map;
+
+// Represents a splat value in Ruby code: *array
+public class BuildSplatInstr extends Instr implements ResultInstr {
+    private Variable result;
+    private Operand array;
+
+    public BuildSplatInstr(Variable result, Operand array) {
+        super(Operation.BUILD_SPLAT);
+        this.result = result;
+        this.array = array;
+    }
+
+    @Override
+    public String toString() {
+        return result + " = *" + array;
+    }
+
+    @Override
+    public Variable getResult() {
+        return result;
+    }
+
+    @Override
+    public void updateResult(Variable v) {
+        this.result = v;
+    }
+
+    public Operand getArray() {
+        return array;
+    }
+
+    @Override
+    public Operand[] getOperands() {
+        return new Operand[] { array };
+    }
+
+    @Override
+    public void simplifyOperands(Map<Operand, Operand> valueMap, boolean force) {
+        array = array.getSimplifiedOperand(valueMap, force);
+    }
+
+    @Override
+    public Instr clone(CloneInfo ii) {
+        return new BuildSplatInstr(ii.getRenamedVariable(result), array.cloneForInlining(ii));
+    }
+
+    @Override
+    public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
+        IRubyObject arrayVal = (IRubyObject) array.retrieve(context, self, currScope, currDynScope, temp);
+        return IRRuntimeHelpers.irSplat(context, arrayVal);
+    }
+
+    @Override
+    public void visit(IRVisitor visitor) {
+        visitor.BuildSplatInstr(this);
+    }
+}
diff --git a/core/src/main/java/org/jruby/ir/instructions/CallBase.java b/core/src/main/java/org/jruby/ir/instructions/CallBase.java
@@ -402,7 +402,7 @@ public String toString() {
 
     public static boolean containsArgSplat(Operand[] arguments) {
         for (Operand argument : arguments) {
-            if (argument instanceof Splat && ((Splat)argument).unsplatArgs) return true;
+            if (argument instanceof Splat) return true;
         }
 
         return false;

diff --git a/core/src/main/java/org/jruby/ir/operands/Splat.java b/core/src/main/java/org/jruby/ir/operands/Splat.java
@@ -17,21 +17,15 @@
 // Further down the line, it could get converted to calls that implement splat semantics
 public class Splat extends Operand implements DepthCloneable {
     final private Operand array;
-    final public boolean unsplatArgs;
 
-    public Splat(Operand array, boolean unsplatArgs) {
+    public Splat(Operand array) {
         super(OperandType.SPLAT);
         this.array = array;
-        this.unsplatArgs = unsplatArgs;
-    }
-
-    public Splat(Operand array) {
-        this(array, false);
     }
 
     @Override
     public String toString() {
-        return (unsplatArgs ? "*(unsplat)" : "*") + array;
+        return "*(unsplat)" + array;
     }
 
     @Override
@@ -46,7 +40,7 @@ public Operand getArray() {
     @Override
     public Operand getSimplifiedOperand(Map<Operand, Operand> valueMap, boolean force) {
         Operand newArray = array.getSimplifiedOperand(valueMap, force);
-        return (newArray == array) ? this : new Splat(newArray, unsplatArgs);
+        return (newArray == array) ? this : new Splat(newArray);
     }
 
     /** Append the list of variables used in this operand to the input list */
@@ -57,19 +51,31 @@ public void addUsedVariables(List<Variable> l) {
 
     /** When fixing up splats in nested closure we need to tweak the operand if it is a LocalVariable */
     public Operand cloneForDepth(int n) {
-        return array instanceof LocalVariable ? new Splat(((LocalVariable) array).cloneForDepth(n), unsplatArgs) : this;
+        return array instanceof LocalVariable ? new Splat(((LocalVariable) array).cloneForDepth(n)) : this;
     }
 
     @Override
     public Operand cloneForInlining(CloneInfo ii) {
-        return hasKnownValue() ? this : new Splat(array.cloneForInlining(ii), unsplatArgs);
+        return hasKnownValue() ? this : new Splat(array.cloneForInlining(ii));
     }
 
     @Override
     public Object retrieve(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp) {
-        IRubyObject arrayVal = (IRubyObject) array.retrieve(context, self, currScope, currDynScope, temp);
-        // SSS FIXME: Some way to specialize this code?
-        return IRRuntimeHelpers.irSplat(context, arrayVal);
+        // Splat is now only used in call arg lists where it is guaranteed that
+        // the splat-arg is an array.
+        //
+        // It is:
+        // - either a result of a args-cat/args-push (which generate an array),
+        // - or a result of a BuildSplatInstr (which also generates an array),
+        // - or a rest-arg that has been received (which also generates an array)
+        //   and is being passed via zsuper.
+        //
+        // In addition, since this only shows up in call args, the array itself is
+        // never modified. The array elements are extracted out and inserted into
+        // a java array. So, a dup is not required either.
+        //
+        // So, besides retrieving the array, nothing more to be done here!
+        return (IRubyObject) array.retrieve(context, self, currScope, currDynScope, temp);
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/persistence/OperandDecoderMap.java b/core/src/main/java/org/jruby/ir/persistence/OperandDecoderMap.java
@@ -50,7 +50,7 @@ public Operand decode(OperandType type) {
             case REGEXP: return decodeRegexp();
             case SCOPE_MODULE: return new ScopeModule(d.decodeInt());
             case SELF: return Self.SELF;
-            case SPLAT: return new Splat(d.decodeOperand(), d.decodeBoolean());
+            case SPLAT: return new Splat(d.decodeOperand());
             case STANDARD_ERROR: return new StandardError();
             case STRING_LITERAL: return new StringLiteral(d.decodeString());
             case SVALUE: return new SValue(d.decodeOperand());

diff --git a/core/src/main/java/org/jruby/ir/persistence/OperandEncoderMap.java b/core/src/main/java/org/jruby/ir/persistence/OperandEncoderMap.java
@@ -93,7 +93,7 @@ public void encode(Operand operand) {
 
     @Override public void Self(Self self) {} // No data
 
-    @Override public void Splat(Splat splat) { encode(splat.getArray()); encoder.encode(splat.unsplatArgs); }
+    @Override public void Splat(Splat splat) { encode(splat.getArray()); }
 
     @Override public void StandardError(StandardError standarderror) {} // No data
 

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -782,6 +782,14 @@ public void BuildRangeInstr(BuildRangeInstr instr) {
         jvmStoreLocal(instr.getResult());
     }
 
+    @Override
+    public void BuildSplatInstr(BuildSplatInstr instr) {
+        jvmMethod().loadContext();
+        visit(instr.getArray());
+        jvmMethod().invokeIRHelper("irSplat", sig(RubyArray.class, ThreadContext.class, IRubyObject.class));
+        jvmStoreLocal(instr.getResult());
+    }
+
     @Override
     public void CallInstr(CallInstr callInstr) {
         IRBytecodeAdapter m = jvmMethod();
@@ -2115,7 +2123,20 @@ public void Self(Self self) {
     public void Splat(Splat splat) {
         jvmMethod().loadContext();
         visit(splat.getArray());
-        jvmMethod().invokeIRHelper("irSplat", sig(RubyArray.class, ThreadContext.class, IRubyObject.class));
+        // Splat is now only used in call arg lists where it is guaranteed that
+        // the splat-arg is an array.
+        //
+        // It is:
+        // - either a result of a args-cat/args-push (which generate an array),
+        // - or a result of a BuildSplatInstr (which also generates an array),
+        // - or a rest-arg that has been received (which also generates an array)
+        //   and is being passed via zsuper.
+        //
+        // In addition, since this only shows up in call args, the array itself is
+        // never modified. The array elements are extracted out and inserted into
+        // a java array. So, a dup is not required either.
+        //
+        // So, besides retrieving the array, nothing more to be done here!
     }
 
     @Override