Skip to content

Commit

Permalink
[IR] Extend WrappedIRClosure to include an explicit self operand
Browse files Browse the repository at this point in the history
* This fix is required for proper inlining of scopes that contain
  blocks so that the self prior-to-inlining can be passed into
  the block after-inlining into another scope.

* With this fix, bench_method_dispatch.rb runs with -Xir.profile=true
  without crashing. Same for bench_fib_recursive, bench_red_black,
  shootout/meteor, shootout/mandelbrot, bench_richards,
  bench_neural_net, and bench_tictactoe.

* There are still known bugs in the inlining code (search for
  FIXMEs or 'This is buggy' comments in the relevant code), but the
  correctness has moved along far enough to enable profiling and
  inlining on a large enough set of programs without crashing or
  yielding incorrect results.
  • Loading branch information
subbuss authored and tedpennings committed Dec 4, 2013
1 parent 37b11f8 commit 4b53d5c
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 25 deletions.
8 changes: 5 additions & 3 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,9 @@ public Operand buildLambda(LambdaNode node, IRScope s) {
// catchUncaughtBreakInLambdas(closure);

Variable lambda = s.getNewTemporaryVariable();
s.addInstr(new BuildLambdaInstr(lambda, closure, node.getPosition()));
// SSS FIXME: Is this the right self here?
WrappedIRClosure lambdaBody = new WrappedIRClosure(getSelf(s), closure);
s.addInstr(new BuildLambdaInstr(lambda, lambdaBody, node.getPosition()));
return lambda;
}

Expand Down Expand Up @@ -2485,7 +2487,7 @@ public Operand buildForIter(final ForNode forNode, IRScope s) {
closure.addInstr(new ReturnInstr(closureRetVal));
}

return new WrappedIRClosure(closure);
return new WrappedIRClosure(getSelf(s), closure);
}

public Operand buildGlobalAsgn(GlobalAsgnNode globalAsgnNode, IRScope s) {
Expand Down Expand Up @@ -2639,7 +2641,7 @@ public Operand buildIter(final IterNode iterNode, IRScope s) {
closure.addInstr(new ReturnInstr(closureRetVal));
}

return new WrappedIRClosure(closure);
return new WrappedIRClosure(getSelf(s), closure);
}

public Operand buildLiteral(LiteralNode literalNode, IRScope s) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ public class BuildLambdaInstr extends Instr implements ResultInstr {
private Variable result;
private Operand[] operands;

public BuildLambdaInstr(Variable lambda, IRClosure lambdaBody, ISourcePosition position) {
public BuildLambdaInstr(Variable lambda, WrappedIRClosure lambdaBody, ISourcePosition position) {
super(Operation.LAMBDA);

this.result = lambda;
this.operands = new Operand[] { new WrappedIRClosure(lambdaBody) };
this.operands = new Operand[] { lambdaBody };
this.position = position;
}

Expand All @@ -46,7 +46,7 @@ public void updateResult(Variable v) {
@Override
public Instr cloneForInlining(InlinerInfo ii) {
// SSS FIXME: This is buggy. The lambda body might have to be cloned depending on cloning context.
return new BuildLambdaInstr(ii.getRenamedVariable(getResult()), getLambdaBody(), position);
return new BuildLambdaInstr(ii.getRenamedVariable(getResult()), (WrappedIRClosure)operands[0], position);
}

@Override
Expand All @@ -69,6 +69,7 @@ public Object interpret(ThreadContext context, DynamicScope currDynScope, IRubyO

IRClosure body = getLambdaBody();
// ENEBO: Now can live nil be passed as block reference?
// SSS FIXME: Should we do the same %self retrieval as in the case of WrappedIRClosure? Or are lambdas special??
return RubyProc.newProc(context.runtime,
(Block) (body == null ? context.runtime.getIRManager().getNil() : operands[0]).retrieve(context, self, currDynScope, temp),
Block.Type.LAMBDA, position);
Expand Down
5 changes: 0 additions & 5 deletions core/src/main/java/org/jruby/ir/operands/Self.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@ public Variable clone(InlinerInfo ii) {
return this;
}

@Override
public Operand cloneForInlining(InlinerInfo ii) {
return ii.getSelfValue(this);
}

@Override
public void visit(IRVisitor visitor) {
visitor.Self(this);
Expand Down
19 changes: 13 additions & 6 deletions core/src/main/java/org/jruby/ir/operands/WrappedIRClosure.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,17 @@
import java.util.List;

public class WrappedIRClosure extends Operand {
private Variable self;
private final IRClosure closure;

public WrappedIRClosure(IRClosure scope) {
this.closure = scope;
public WrappedIRClosure(Variable self, IRClosure closure) {
this.self = self;
this.closure = closure;
}

@Override
public void addUsedVariables(List<Variable> l) {
/* Nothing to o */
l.add(self);
}

public IRClosure getClosure() {
Expand All @@ -35,19 +37,24 @@ public boolean canCopyPropagate() {

@Override
public String toString() {
return closure.toString();
return self + ":" + closure.toString();
}

@Override
public Operand cloneForInlining(InlinerInfo ii) {
return new WrappedIRClosure(closure.cloneForInlining(ii));
return new WrappedIRClosure(ii.getRenamedVariable(self), closure.cloneForInlining(ii));
}

@Override
public Object retrieve(ThreadContext context, IRubyObject self, DynamicScope currDynScope, Object[] temp) {
BlockBody body = closure.getBlockBody();
closure.getStaticScope().determineModule();
Binding binding = context.currentBinding(self, currDynScope);

// 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, currDynScope, temp);
Binding binding = context.currentBinding(selfVal, currDynScope);

return new Block(body, binding);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,15 @@ public void inlineMethod(IRScope scope, RubyModule implClass, int classToken, Ba
}

// Host method data init
InlinerInfo ii = new InlinerInfo(call, cfg);
Operand callReceiver = call.getReceiver();
Variable callReceiverVar;
if (callReceiver instanceof Variable) {
callReceiverVar = (Variable)callReceiver;
} else {
callReceiverVar = hostScope.getNewTemporaryVariable();
}

InlinerInfo ii = new InlinerInfo(call, cfg, callReceiverVar);

// Inlinee method data init
CFG methodCFG = scope.getCFG();
Expand Down Expand Up @@ -170,6 +178,9 @@ public void inlineMethod(IRScope scope, RubyModule implClass, int classToken, Ba
BasicBlock destination = e.getDestination().getData();
if (destination != mExit) {
BasicBlock dstBB = ii.getRenamedBB(destination);
if (callReceiver != callReceiverVar) {
dstBB.insertInstr(new CopyInstr(callReceiverVar, callReceiver));
}
if (!ii.canMapArgsStatically()) {
// SSS FIXME: This is buggy!
// This code has to mimic whatever CallBase.prepareArguments does!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class InlinerInfo {
private Map<Variable, Variable> varRenameMap;
private Map<BasicBlock, BasicBlock> bbRenameMap;
private List yieldSites;
private Operand callReceiver;
private Variable callReceiver;
private String inlineVarPrefix;

private CloneMode cloneMode;
Expand Down Expand Up @@ -76,12 +76,12 @@ public InlinerInfo(CFG c) {
this.argsArray = null;
}

public InlinerInfo(CallBase call, CFG c) {
public InlinerInfo(CallBase call, CFG c, Variable callReceiver) {
init(c);
this.cloneMode = CloneMode.METHOD_INLINE;
this.call = call;
this.callArgs = call.getCallArgs();
this.callReceiver = call.getReceiver();
this.callReceiver = callReceiver;
this.canMapArgsStatically = !containsSplat(callArgs);
this.argsArray = this.canMapArgsStatically ? null : getInlineHostScope().getNewTemporaryVariable();
synchronized(globalInlineCount) {
Expand Down Expand Up @@ -158,6 +158,12 @@ public void setupYieldArgsAndYieldResult(YieldInstr yi, BasicBlock yieldBB, Arit
}

public Variable getRenamedVariable(Variable v) {
// Special case for %self
if (v instanceof Self) {
return cloneMode == CloneMode.NORMAL_CLONE ? v : callReceiver;
}

// Everything else
Variable newVar = this.varRenameMap.get(v);
if (newVar == null) {
switch (cloneMode) {
Expand Down Expand Up @@ -187,6 +193,7 @@ public Variable getRenamedVariable(Variable v) {
LocalVariable l_newVar = (LocalVariable)newVar;
if (l_v.getScopeDepth() != l_newVar.getScopeDepth()) newVar = l_newVar.cloneForDepth(l_v.getScopeDepth());
}

return newVar;
}

Expand Down Expand Up @@ -240,10 +247,6 @@ public Operand getArg(int argIndex, boolean restOfArgArray) {
}
}

public Operand getSelfValue(Self self) {
return cloneMode == CloneMode.NORMAL_CLONE ? self : callReceiver;
}

public Operand getCallClosure() {
return call.getClosureArg(hostCFG.getScope().getManager().getNil());
}
Expand Down

0 comments on commit 4b53d5c

Please sign in to comment.