Skip to content

Commit

Permalink
First tweaks to make CallInstr know about kwargs structure.
Browse files Browse the repository at this point in the history
This change does the following:

* Actually marks the hash args's Hash as being a kwargs.
* Emits that Hash directly into the instr rather than indirected
  through a temp var.
* Modifies Hash to use new HashPair operand and array rather than
  opaque List of KeyValuePair (unrelated to kwargs work).

No emitted code is really different at this point, other than the
Hash being local to the CallInstr and marks as a kwargs.
headius committed Sep 16, 2015
1 parent 443c18a commit ba0df56
Showing 9 changed files with 228 additions and 63 deletions.
34 changes: 29 additions & 5 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
Original file line number Diff line number Diff line change
@@ -602,12 +602,24 @@ protected Operand[] buildCallArgs(Node args) {
case ARRAYNODE: {
Node[] children = ((ListNode) args).children();
int numberOfArgs = children.length;

if (numberOfArgs == 0) return Operand.EMPTY_ARRAY;

Operand[] builtArgs = new Operand[numberOfArgs];
boolean hasAssignments = args.containsVariableAssignment();

for (int i = 0; i < numberOfArgs; i++) {
builtArgs[i] = buildWithOrder(children[i], hasAssignments);
// omit last argument; if it is a literal hash, we need to mark it and keep it direct below
int i = 0;
for (; i < numberOfArgs; i++) {
Node curArg = children[i];
if (i == numberOfArgs - 1 && curArg instanceof HashNode) {
// last arg is a literal hash, put it directly in instr
builtArgs[i] = buildHashArgs((HashNode) curArg);
} else {
builtArgs[i] = buildWithOrder(children[i], hasAssignments);
}
}

return builtArgs;
}
case SPLATNODE:
@@ -1001,7 +1013,7 @@ public Operand buildCall(CallNode callNode) {
Operand[] args = setupCallArgs(callArgsNode);
Operand block = setupCallClosure(callNode.getIterNode());
Variable callResult = createTemporaryVariable();
CallInstr callInstr = CallInstr.create(scope, callResult, callNode.getName(), receiver, args, block);
CallInstr callInstr = CallInstr.create(scope, CallType.NORMAL, callResult, callNode.getName(), receiver, args, block);

// This is to support the ugly Proc.new with no block, which must see caller's frame
if ( callNode.getName().equals("new") &&
@@ -2494,6 +2506,14 @@ public Operand buildGlobalVar(GlobalVarNode node) {
}

public Operand buildHash(HashNode hashNode) {
return buildHash(hashNode, false);
}

public Operand buildHashArgs(HashNode hashNode) {
return buildHash(hashNode, true);
}

private Operand buildHash(HashNode hashNode, boolean isKwargs) {
List<KeyValuePair<Operand, Operand>> args = new ArrayList<>();
boolean hasAssignments = hashNode.containsVariableAssignment();
Variable hash = null;
@@ -2504,7 +2524,7 @@ public Operand buildHash(HashNode hashNode) {

if (key == null) { // Splat kwarg [e.g. {**splat1, a: 1, **splat2)]
if (hash == null) { // No hash yet. Define so order is preserved.
hash = copyAndReturnValue(new Hash(args));
hash = copyAndReturnValue(new Hash(args, isKwargs));
args = new ArrayList<>(); // Used args but we may find more after the splat so we reset
} else if (!args.isEmpty()) {
addInstr(new RuntimeHelperCall(hash, MERGE_KWARGS, new Operand[] { hash, new Hash(args) }));
@@ -2521,7 +2541,11 @@ public Operand buildHash(HashNode hashNode) {
}

if (hash == null) { // non-**arg ordinary hash
hash = copyAndReturnValue(new Hash(args));
if (isKwargs) {
return new Hash(args, isKwargs);
} else {
hash = copyAndReturnValue(new Hash(args));
}
} else if (!args.isEmpty()) { // ordinary hash values encountered after a **arg
addInstr(new RuntimeHelperCall(hash, MERGE_KWARGS, new Operand[] { hash, new Hash(args) }));
}
38 changes: 26 additions & 12 deletions core/src/main/java/org/jruby/ir/instructions/CallBase.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.jruby.ir.instructions;

import org.jruby.RubyArray;
import org.jruby.RubyHash;
import org.jruby.ir.IRScope;
import org.jruby.ir.Operation;
import org.jruby.ir.operands.*;
@@ -27,6 +28,7 @@ public abstract class CallBase extends NOperandInstr implements ClosureAccepting
protected String name;
protected CallSite callSite;
protected int argsCount;
protected boolean kwargs;
protected boolean hasClosure;

private boolean flagsComputed;
@@ -38,12 +40,18 @@ public abstract class CallBase extends NOperandInstr implements ClosureAccepting
private boolean procNew;
private boolean potentiallyRefined;

protected CallBase(Operation op, CallType callType, String name, Operand receiver, Operand[] args, Operand closure,
boolean potentiallyRefined) {
super(op, getOperands(receiver, args, closure));
protected CallBase(Operation op, CallType callType, String name, Operand receiver, Operand[] args,
Operand closure, boolean potentiallyRefined) {
this(op, callType, name, receiver, args, false, closure, potentiallyRefined);
}

protected CallBase(Operation op, CallType callType, String name, Operand receiver, Operand[] args, boolean hasKwargs,
Operand closure, boolean potentiallyRefined) {
super(op, buildAllArgs(receiver, args, closure));

this.callSiteId = callSiteCounter++;
argsCount = args.length;
this.kwargs = hasKwargs;
hasClosure = closure != null;
this.name = name;
this.callType = callType;
@@ -78,11 +86,7 @@ public void encode(IRWriterEncoder e) {
// FIXME: Convert this to some Signature/Arity method
// -0 is not possible so we add 1 to arguments with closure so we get a valid negative value.
private int calculateArity() {
return hasClosure ? -1*(argsCount + 1) : argsCount;
}

private static Operand[] getOperands(Operand receiver, Operand[] arguments, Operand closure) {
return buildAllArgs(receiver, arguments, closure);
return hasClosure ? -1 * (argsCount + 1) : argsCount;
}

public String getName() {
@@ -91,7 +95,7 @@ public String getName() {

/** From interface ClosureAcceptingInstr */
public Operand getClosureArg() {
return hasClosure ? operands[argsCount + 1] : null;
return hasClosure ? operands[argsCount + (kwargs ? 1 : 0) + 1] : null;
}

public Operand getClosureArg(Operand ifUnspecified) {
@@ -115,6 +119,11 @@ public int getArgsCount() {
return argsCount;
}

// FIXME: Maybe rename this.
public Hash getKwargs() {
return kwargs ? (Hash)operands[REQUIRED_OPERANDS + argsCount] : null;
}

// Warning: Potentially expensive. Analysis should be written around retrieving operands.
public Operand[] getCallArgs() {
Operand[] callArgs = new Operand[argsCount];
@@ -124,6 +133,10 @@ public Operand[] getCallArgs() {
return callArgs;
}

public HashPair getKwarg(int i) {
return (HashPair)operands[1 + argsCount + i];
}

public CallSite getCallSite() {
return callSite;
}
@@ -393,19 +406,20 @@ public static boolean containsArgSplat(Operand[] arguments) {

private final static int REQUIRED_OPERANDS = 1;
private static Operand[] buildAllArgs(Operand receiver, Operand[] callArgs, Operand closure) {
Operand[] allArgs = new Operand[callArgs.length + REQUIRED_OPERANDS + (closure != null ? 1 : 0)];
Operand[] allArgs = new Operand[REQUIRED_OPERANDS + callArgs.length + (closure != null ? 1 : 0)];

assert receiver != null : "RECEIVER is null";


allArgs[0] = receiver;

for (int i = 0; i < callArgs.length; i++) {
assert callArgs[i] != null : "ARG " + i + " is null";

allArgs[i + REQUIRED_OPERANDS] = callArgs[i];
allArgs[REQUIRED_OPERANDS + i] = callArgs[i];
}

if (closure != null) allArgs[callArgs.length + REQUIRED_OPERANDS] = closure;
if (closure != null) allArgs[REQUIRED_OPERANDS + callArgs.length] = closure;

return allArgs;
}
11 changes: 9 additions & 2 deletions core/src/main/java/org/jruby/ir/instructions/CallInstr.java
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@
import org.jruby.ir.instructions.specialized.OneOperandArgBlockCallInstr;
import org.jruby.ir.instructions.specialized.OneOperandArgNoBlockCallInstr;
import org.jruby.ir.instructions.specialized.ZeroOperandArgNoBlockCallInstr;
import org.jruby.ir.operands.Hash;
import org.jruby.ir.operands.Operand;
import org.jruby.ir.operands.Variable;
import org.jruby.ir.persistence.IRReaderDecoder;
@@ -43,10 +44,9 @@ public static CallInstr create(IRScope scope, CallType callType, Variable result
}
}

return new CallInstr(callType, result, name, receiver, args, closure, isPotentiallyRefined);
return new CallInstr(Operation.CALL, callType, result, name, receiver, args, closure, isPotentiallyRefined);
}


public CallInstr(CallType callType, Variable result, String name, Operand receiver, Operand[] args, Operand closure,
boolean potentiallyRefined) {
this(Operation.CALL, callType, result, name, receiver, args, closure, potentiallyRefined);
@@ -79,6 +79,8 @@ public static CallInstr decode(IRReaderDecoder d) {
if (RubyInstanceConfig.IR_READING_DEBUG) System.out.println("decodeCall - receiver: " + receiver);
int argsCount = d.decodeInt();
if (RubyInstanceConfig.IR_READING_DEBUG) System.out.println("decodeCall - # of args: " + argsCount);
int kwargsCount = d.decodeInt();
if (RubyInstanceConfig.IR_READING_DEBUG) System.out.println("decodeCall - # of kwargs: " + argsCount);
boolean hasClosureArg = argsCount < 0;
int argsLength = hasClosureArg ? (-1 * (argsCount + 1)) : argsCount;
if (RubyInstanceConfig.IR_READING_DEBUG) System.out.println("decodeCall - # of args(2): " + argsLength);
@@ -89,6 +91,11 @@ public static CallInstr decode(IRReaderDecoder d) {
args[i] = d.decodeOperand();
}

Operand[] kwargs = new Operand[kwargsCount];
for (int i = 0; i < kwargsCount; i++) {
kwargs[i] = d.decodeOperand();
}

Operand closure = hasClosureArg ? d.decodeOperand() : null;
if (RubyInstanceConfig.IR_READING_DEBUG) System.out.println("before result");
Variable result = d.decodeVariable();
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/instructions/Instr.java
Original file line number Diff line number Diff line change
@@ -81,7 +81,7 @@ public String toString() {
String[] extraArgs = toStringNonOperandArgs();
if (extraArgs.length >= 1) {
if (operands.length > 0) buf.append(' ');
buf.append(';');
buf.append("; ");
toArgList(buf, extraArgs);
}
buf.append(')');
Original file line number Diff line number Diff line change
@@ -62,6 +62,7 @@
import org.jruby.runtime.ivars.VariableAccessor;
import org.jruby.runtime.opto.ConstantCache;

import java.util.Arrays;
import java.util.Stack;

/**
82 changes: 49 additions & 33 deletions core/src/main/java/org/jruby/ir/operands/Hash.java
Original file line number Diff line number Diff line change
@@ -23,13 +23,27 @@
// Further down the line, this hash could get converted to calls
// that actually build the hash
public class Hash extends Operand {
final public List<KeyValuePair<Operand, Operand>> pairs;
final public HashPair[] pairs;

// Is this a hash used to represent a keyword hash to be setup for ZSuper?
// SSS FIXME: Quick hack for now - this should probably be done with an overloaded operand.
final public boolean isKWArgsHash;

public Hash(List<KeyValuePair<Operand, Operand>> pairs) {
this(pairs, false);
}

public Hash(List<KeyValuePair<Operand, Operand>> pairs, boolean isKWArgsHash) {
HashPair[] newPairs = new HashPair[pairs.size()];
for (int i = 0; i < pairs.size(); i++) {
newPairs[i] = new HashPair(pairs.get(i).getKey(), pairs.get(i).getValue());
}

this.pairs = newPairs;
this.isKWArgsHash = isKWArgsHash;
}

public Hash(HashPair[] pairs, boolean isKWArgsHash) {
super();

this.pairs = pairs;
@@ -41,17 +55,13 @@ public OperandType getOperandType() {
return OperandType.HASH;
}

public Hash(List<KeyValuePair<Operand, Operand>> pairs) {
this(pairs, false);
}

public boolean isBlank() {
return pairs == null || pairs.isEmpty();
return pairs == null || pairs.length == 0;
}

@Override
public boolean hasKnownValue() {
for (KeyValuePair<Operand, Operand> pair : pairs) {
for (HashPair pair : pairs) {
if (!pair.getKey().hasKnownValue() || !pair.getValue().hasKnownValue())
return false;
}
@@ -61,10 +71,13 @@ public boolean hasKnownValue() {

@Override
public Operand getSimplifiedOperand(Map<Operand, Operand> valueMap, boolean force) {
List<KeyValuePair<Operand, Operand>> newPairs = new java.util.ArrayList<>();
for (KeyValuePair<Operand, Operand> pair : pairs) {
newPairs.add(new KeyValuePair(pair.getKey().getSimplifiedOperand(valueMap, force), pair
.getValue().getSimplifiedOperand(valueMap, force)));
HashPair[] newPairs = new HashPair[pairs.length];
for (int i = 0; i < pairs.length; i++) {
HashPair pair = pairs[i];
newPairs[i] =
new HashPair(
pair.getKey().getSimplifiedOperand(valueMap, force),
pair.getValue().getSimplifiedOperand(valueMap, force));
}

return new Hash(newPairs, isKWArgsHash);
@@ -73,16 +86,17 @@ public Operand getSimplifiedOperand(Map<Operand, Operand> valueMap, boolean forc
/** Append the list of variables used in this operand to the input list */
@Override
public void addUsedVariables(List<Variable> l) {
for (KeyValuePair<Operand, Operand> pair : pairs) {
for (HashPair pair : pairs) {
pair.getKey().addUsedVariables(l);
pair.getValue().addUsedVariables(l);
}
}

public Operand cloneForLVarDepth(int newDepth) {
List<KeyValuePair<Operand, Operand>> newPairs = new java.util.ArrayList<>();
for (KeyValuePair<Operand, Operand> pair : pairs) {
newPairs.add(new KeyValuePair(pair.getKey(), ((DepthCloneable) pair.getValue()).cloneForDepth(newDepth)));
HashPair[] newPairs = new HashPair[pairs.length];
for (int i = 0; i < pairs.length; i++) {
HashPair pair = pairs[i];
newPairs[i] = new HashPair(pair.getKey(), ((DepthCloneable) pair.getValue()).cloneForDepth(newDepth));
}
return new Hash(newPairs, isKWArgsHash);
}
@@ -92,30 +106,32 @@ public Operand cloneForInlining(CloneInfo ii) {
if (hasKnownValue())
return this;

List<KeyValuePair<Operand, Operand>> newPairs = new java.util.ArrayList<>();
for (KeyValuePair<Operand, Operand> pair : pairs) {
newPairs.add(new KeyValuePair(pair.getKey().cloneForInlining(ii), pair.getValue().cloneForInlining(ii)));
HashPair[] newPairs = new HashPair[pairs.length];
for (int i = 0; i < pairs.length; i++) {
HashPair pair = pairs[i];
newPairs[i] = new HashPair(pair.getKey().cloneForInlining(ii), pair.getValue().cloneForInlining(ii));
}
return new Hash(newPairs, isKWArgsHash);
}

// TODO: For small literal hashes, we should use RubyHash.newSmallHash plus appropriate inserts to avoid unnecessary buckets
@Override
public Object retrieve(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp) {
Ruby runtime = context.runtime;
RubyHash hash;
Iterator<KeyValuePair<Operand, Operand>> it = pairs.iterator();
int i = 0;

if (isKWArgsHash && pairs.get(0).getKey() == Symbol.KW_REST_ARG_DUMMY) {
if (isKWArgsHash && pairs.length > 0 && pairs[0].getKey() == Symbol.KW_REST_ARG_DUMMY) {
// Dup the rest args hash and use that as the basis for inserting the non-rest args
hash = ((RubyHash) pairs.get(0).getValue().retrieve(context, self, currScope, currDynScope, temp)).dupFast(context);
hash = ((RubyHash) pairs[0].getValue().retrieve(context, self, currScope, currDynScope, temp)).dupFast(context);
// Skip the first pair
it.next();
i = 1;
} else {
hash = RubyHash.newHash(runtime);
}

while (it.hasNext()) {
KeyValuePair<Operand, Operand> pair = it.next();
for (; i < pairs.length; i++) {
HashPair pair = pairs[i];
IRubyObject key = (IRubyObject) pair.getKey().retrieve(context, self, currScope, currDynScope, temp);
IRubyObject value = (IRubyObject) pair.getValue().retrieve(context, self, currScope, currDynScope, temp);

@@ -133,8 +149,8 @@ public void visit(IRVisitor visitor) {
@Override
public void encode(IRWriterEncoder e) {
super.encode(e);
e.encode(pairs.size());
for (KeyValuePair<Operand, Operand> pair: pairs) {
e.encode(pairs.length);
for (HashPair pair: pairs) {
e.encode(pair.getKey());
e.encode(pair.getValue());
}
@@ -143,33 +159,33 @@ public void encode(IRWriterEncoder e) {

public static Hash decode(IRReaderDecoder d) {
int size = d.decodeInt();
List<KeyValuePair<Operand, Operand>> pairs = new ArrayList<>(size);

HashPair[] newPairs = new HashPair[size];
for (int i = 0; i < size; i++) {
pairs.add(new KeyValuePair(d.decodeOperand(), d.decodeOperand()));
newPairs[i] = new HashPair(d.decodeOperand(), d.decodeOperand());
}

return new Hash(pairs, d.decodeBoolean());
return new Hash(newPairs, d.decodeBoolean());
}

@Override
public String toString() {
StringBuilder builder = new StringBuilder();
if (isKWArgsHash) builder.append("KW");
builder.append("{");
if (!isBlank()) {
int pairCount = pairs.size();
int pairCount = pairs.length;
for (int i = 0; i < pairCount; i++) {
if (i > 0) {
builder.append(", ");
}
builder.append(pairs.get(i));
builder.append(pairs[i]);
}
}
builder.append("}");
return builder.toString();
}

public List<KeyValuePair<Operand, Operand>> getPairs() {
public HashPair[] getPairs() {
return pairs;
}
}
102 changes: 102 additions & 0 deletions core/src/main/java/org/jruby/ir/operands/HashPair.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
package org.jruby.ir.operands;

import org.jruby.ir.persistence.IRReaderDecoder;
import org.jruby.ir.persistence.IRWriterEncoder;
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 org.jruby.util.KeyValuePair;

import java.util.List;
import java.util.Map;

/**
* Represents a key/value pair in a literal hash (currently only used for hash/keyword args).
*/
public class HashPair extends Operand implements Map.Entry<Operand, Operand> {
public final Operand key;
public final Operand value;

public HashPair(Operand key, Operand value) {
super();

this.key = key;
this.value = value;
}

@Override
public OperandType getOperandType() {
return OperandType.HASH_PAIR;
}

@Override
public boolean hasKnownValue() {
if (!getKey().hasKnownValue() || !getValue().hasKnownValue())
return false;

return true;
}

@Override
public Operand getSimplifiedOperand(Map<Operand, Operand> valueMap, boolean force) {
return new HashPair(
getKey().getSimplifiedOperand(valueMap, force),
getValue().getSimplifiedOperand(valueMap, force));
}

/** Append the list of variables used in this operand to the input list */
@Override
public void addUsedVariables(List<Variable> l) {
getKey().addUsedVariables(l);
getValue().addUsedVariables(l);
}

public Operand cloneForLVarDepth(int newDepth) {
return new HashPair(
getKey(),
((DepthCloneable) getValue()).cloneForDepth(newDepth));
}

@Override
public Operand cloneForInlining(CloneInfo ii) {
if (hasKnownValue())
return this;

return new HashPair(
getKey().cloneForInlining(ii),
getValue().cloneForInlining(ii));
}

@Override
public void encode(IRWriterEncoder e) {
super.encode(e);
e.encode(getKey());
e.encode(getValue());
}

public static HashPair decode(IRReaderDecoder d) {
return new HashPair(d.decodeOperand(), d.decodeOperand());
}

@Override
public String toString() {
return getKey() + "=>" + getValue();
}

@Override
public Operand getKey() {
return key;
}

@Override
public Operand getValue() {
return value;
}

@Override
public Operand setValue(Operand value) {
throw new UnsupportedOperationException();
}
}
3 changes: 2 additions & 1 deletion core/src/main/java/org/jruby/ir/operands/OperandType.java
Original file line number Diff line number Diff line change
@@ -47,7 +47,8 @@ public enum OperandType {
WRAPPED_IR_CLOSURE((byte) 'w'),
FROZEN_STRING((byte) 'z'),
NULL_BLOCK((byte) 'o'),
FILENAME((byte) 'm')
FILENAME((byte) 'm'),
HASH_PAIR((byte) ':')
;

private final byte coded;
18 changes: 9 additions & 9 deletions core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
Original file line number Diff line number Diff line change
@@ -2035,28 +2035,28 @@ public void UnboxedFloat(org.jruby.ir.operands.UnboxedFloat flote) {

@Override
public void Hash(Hash hash) {
List<KeyValuePair<Operand, Operand>> pairs = hash.getPairs();
Iterator<KeyValuePair<Operand, Operand>> iter = pairs.iterator();
boolean kwargs = hash.isKWArgsHash && pairs.get(0).getKey() == Symbol.KW_REST_ARG_DUMMY;
HashPair[] pairs = hash.getPairs();
int i = 0;
boolean kwargs = hash.isKWArgsHash && pairs[0].getKey() == Symbol.KW_REST_ARG_DUMMY;

jvmMethod().loadContext();
if (kwargs) {
visit(pairs.get(0).getValue());
visit(pairs[0].getValue());
jvmAdapter().checkcast(p(RubyHash.class));

iter.next();
i++;
}

for (; iter.hasNext() ;) {
KeyValuePair<Operand, Operand> pair = iter.next();
for (; i < pairs.length; i++) {
HashPair pair = pairs[i];
visit(pair.getKey());
visit(pair.getValue());
}

if (kwargs) {
jvmMethod().kwargsHash(pairs.size() - 1);
jvmMethod().kwargsHash(pairs.length - 1);
} else {
jvmMethod().hash(pairs.size());
jvmMethod().hash(pairs.length);
}
}

0 comments on commit ba0df56

Please sign in to comment.