Skip to content

Commit

Permalink
Eliminate special-case break/nonlocal return handling for lambdas
Browse files Browse the repository at this point in the history 
* Lambdas need to trap breaks/nonlocal-returns and terminate them.
  However, whether a block becomes a lambda or not is determined
  at runtime. So, only blocks that becomes lambdas need a try/catch
  to trap these breaks/nonlocal-returns. So far, in the interp,
  we used to tackle these by adding the try/catch at runtime on
  first execution.

* This patch eliminates that runtime check and unconditionally
  adds the try/catch for all blocks (lambdas or not). Did this by
  uncommenting the unconditional handling that was there in
  IRBuilder.

  To make sure everything continued to work fine, fixed non-local
  return handling in IRReturnHelpers to ignore them if the
  blockType is not null (methods) or a lambda. This ensures
  that independent of what exists on the scope-stack, we only test
  for a matching non-local returns in lambdas or method scopes.

* Had to fix up addition of global-ensure-blocks in store-local-var
  analysis to only add a GEB if one didn't already exist.

* Eliminated all lambda-specific tests while preparing instructions
  for interpretation/JITting.

* Unrelated:
  - Fixed toStringOutput of BacktickInstr
  • Loading branch information
@subbuss
subbuss committed Oct 14, 2014
1 parent 905db5f commit 4a5d818
Show file tree
Hide file tree
Showing 14 changed files with 66 additions and 109 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/internal/runtime/methods/InterpretedIRMethod.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public void ensureInstrsReady() {
// SSS FIXME: Move this out of here to some other place?
// Prepare method if not yet done so we know if the method has an explicit/implicit call protocol
if (method.getInstrsForInterpretation() == null) {
InterpreterContext context = method.prepareForInterpretation(false);
InterpreterContext context = method.prepareForInterpretation();
this.pushScope = !context.getFlags().contains(IRFlags.DYNSCOPE_ELIMINATED);
}
}
Expand Down
44 changes: 29 additions & 15 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ public void addInstr(Instr i) {
instrs.add(i);
}

public void addInstrAtBeginning(Instr i) {
instrs.add(0, i);
}

public void emitBody(IRBuilder b, IRScope s) {
b.addInstr(s, new LabelInstr(start));
for (Instr i: instrs) {
Expand Down Expand Up @@ -293,6 +297,16 @@ public void addInstr(IRScope s, Instr i) {
}
}

public void addInstrAtBeginning(IRScope s, Instr i) {
// If we are building an ensure body, stash the instruction
// in the ensure body's list. If not, add it to the scope directly.
if (ensureBodyBuildStack.empty()) {
s.addInstrAtBeginning(i);
} else {
ensureBodyBuildStack.peek().addInstrAtBeginning(i);
}
}

private Operand getImplicitBlockArg(IRScope s) {
int n = 0;
while (s != null && s instanceof IRClosure) {
Expand Down Expand Up @@ -505,8 +519,7 @@ public Operand buildLambda(LambdaNode node, IRScope s) {
// can be U_NIL if the node is an if node with returns in both branches.
if (closureRetVal != U_NIL) closureBuilder.addInstr(closure, new ReturnInstr(closureRetVal));

// Added as part of 'prepareForInterpretation' code.
// handleBreakAndReturnsInLambdas(closure);
handleBreakAndReturnsInLambdas(closure);

Variable lambda = s.createTemporaryVariable();
// SSS FIXME: Is this the right self here?
Expand Down Expand Up @@ -891,8 +904,8 @@ private void handleNonlocalReturnInMethod(IRScope s) {
//
// Add label and marker instruction in reverse order to the beginning
// so that the label ends up being the first instr.
s.addInstrAtBeginning(new ExceptionRegionStartMarkerInstr(gebLabel));
s.addInstrAtBeginning(new LabelInstr(rBeginLabel));
addInstrAtBeginning(s, new ExceptionRegionStartMarkerInstr(gebLabel));
addInstrAtBeginning(s, new LabelInstr(rBeginLabel));
addInstr(s, new ExceptionRegionEndMarkerInstr());

// Receive exceptions (could be anything, but the handler only processes IRReturnJumps)
Expand Down Expand Up @@ -1970,19 +1983,13 @@ public void buildMultipleAsgn19Assignment(final MultipleAsgn19Node multipleAsgnN
}
}

/* ------------------------------------------------------------------
* This code is added on demand at runtime in the interpreter code.
* For JIT, this may have to be added always!
// These two methods could have been DRY-ed out if we had closures.
// For now, just duplicating code.
private void handleBreakAndReturnsInLambdas(IRClosure s) {
Label rBeginLabel = s.getNewLabel();
Label rEndLabel = s.getNewLabel();
Label rescueLabel = s.getNewLabel();
Label rescueLabel = Label.GLOBAL_ENSURE_BLOCK_LABEL;

// protect the entire body as it exists now with the global ensure block
s.addInstrAtBeginning(new ExceptionRegionStartMarkerInstr(rescueLabel));
addInstrAtBeginning(s, new ExceptionRegionStartMarkerInstr(rescueLabel));
addInstr(s, new ExceptionRegionEndMarkerInstr());

// Receive exceptions (could be anything, but the handler only processes IRBreakJumps)
Expand All @@ -1992,14 +1999,13 @@ private void handleBreakAndReturnsInLambdas(IRClosure s) {

// Handle break using runtime helper
// --> IRRuntimeHelpers.handleBreakAndReturnsInLambdas(context, scope, bj, blockType)
Variable ret = createTemporaryVariable();
addInstr(s, new RuntimeHelperCall(ret, "handleBreakAndReturnsInLambdas", new Operand[]{exc} ));
Variable ret = s.createTemporaryVariable();
addInstr(s, new RuntimeHelperCall(ret, RuntimeHelperCall.Methods.HANDLE_BREAK_AND_RETURNS_IN_LAMBDA, new Operand[]{exc} ));
addInstr(s, new ReturnInstr(ret));

// End
addInstr(s, new LabelInstr(rEndLabel));
}
* ------------------------------------------------------------------ */

public void receiveMethodArgs(final ArgsNode argsNode, IRScope s) {
receiveArgs(argsNode, s);
Expand Down Expand Up @@ -2524,6 +2530,14 @@ public Operand buildIter(final IterNode iterNode, IRScope s) {
closureBuilder.addInstr(closure, new ReturnInstr(closureRetVal));
}

// Always add break/return handling even though this
// is only required for lambdas, but we don't know at this time,
// if this is a lambda or not.
//
// SSS FIXME: At a later time, see if we can optimize this and
// do this on demand.
closureBuilder.handleBreakAndReturnsInLambdas(closure);

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

Expand Down
45 changes: 0 additions & 45 deletions core/src/main/java/org/jruby/ir/IRClosure.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ public class IRClosure extends IRScope {

private Arity arity;
private int argumentType;
public boolean addedGEBForUncaughtBreaks;

/** Added for interp/JIT purposes */
private BlockBody body;
Expand Down Expand Up @@ -78,7 +77,6 @@ protected IRClosure(IRClosure c, IRScope lexicalParent, int closureId, String fu
} else {
this.body = new InterpretedIRBlockBody(this, c.body.arity(), c.body.getArgumentType());
}
this.addedGEBForUncaughtBreaks = false;
this.blockArgs = new ArrayList<Operand>();
this.arity = c.arity;
}
Expand Down Expand Up @@ -292,7 +290,6 @@ protected IRClosure cloneForInlining(CloneInfo ii, IRClosure clone) {
// FIXME: This is fragile. Untangle this state.
// Why is this being copied over to InterpretedIRBlockBody?
clone.setParameterList(this.parameterList);
clone.addedGEBForUncaughtBreaks = this.addedGEBForUncaughtBreaks;
clone.isBeginEndBlock = this.isBeginEndBlock;

SimpleCloneInfo clonedII = ii.cloneForCloningClosure(clone);
Expand Down Expand Up @@ -327,48 +324,6 @@ public IRClosure cloneForInlining(CloneInfo ii) {
return cloneForInlining(ii, clonedClosure);
}

// Add a global-ensure-block to catch uncaught breaks
// This is usually required only if this closure is being
// used as a lambda, but it is safe to add this for any closure

protected boolean addGEBForUncaughtBreaks() {
// Nothing to do if already done
if (addedGEBForUncaughtBreaks) {
return false;
}

CFG cfg = cfg();
BasicBlock geb = cfg.getGlobalEnsureBB();
if (geb == null) {
geb = new BasicBlock(cfg, new Label("_GLOBAL_ENSURE_BLOCK", 0));
Variable exc = createTemporaryVariable();
geb.addInstr(new ReceiveJRubyExceptionInstr(exc)); // JRuby implementation exception
// Handle uncaught break and non-local returns using runtime helpers
Variable ret = createTemporaryVariable();
geb.addInstr(new RuntimeHelperCall(ret,
RuntimeHelperCall.Methods.HANDLE_BREAK_AND_RETURNS_IN_LAMBDA, new Operand[]{exc} ));
geb.addInstr(new ReturnInstr(ret));
cfg.addGlobalEnsureBB(geb);
} else {
// SSS FIXME: Assumptions:
//
// First instr is a 'ReceiveExceptionBase'
// Last instr is a 'ThrowExceptionInstr' -- replaced by handleBreakAndReturnsInLambdas

List<Instr> instrs = geb.getInstrs();
Variable exc = ((ReceiveExceptionBase)instrs.get(0)).getResult();
Variable ret = createTemporaryVariable();
instrs.set(instrs.size()-1, new RuntimeHelperCall(ret,
RuntimeHelperCall.Methods.HANDLE_BREAK_AND_RETURNS_IN_LAMBDA, new Operand[]{exc} ));
geb.addInstr(new ReturnInstr(ret));
}

// Update scope
addedGEBForUncaughtBreaks = true;

return true;
}

@Override
public void setName(String name) {
// We can distinguish closures only with parent scope name
Expand Down
31 changes: 4 additions & 27 deletions core/src/main/java/org/jruby/ir/IRScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ private void optimizeSimpleScopes() {
}
}

private void initScope(boolean isLambda, boolean jitMode) {
private void initScope(boolean jitMode) {
// Reset linearization, if any exists
resetLinearizationData();

Expand All @@ -618,15 +618,6 @@ private void initScope(boolean isLambda, boolean jitMode) {
buildCFG();
}

if (isLambda) {
// Add a global ensure block to catch uncaught breaks
// and throw a LocalJumpError.
if (((IRClosure)this).addGEBForUncaughtBreaks()) {
resetState();
computeScopeFlags();
}
}

runCompilerPasses(getManager().getCompilerPasses(this));

if (!jitMode && RubyInstanceConfig.IR_COMPILER_PASSES == null) {
Expand All @@ -640,8 +631,8 @@ private void initScope(boolean isLambda, boolean jitMode) {
}

/** Run any necessary passes to get the IR ready for interpretation */
public synchronized InterpreterContext prepareForInterpretation(boolean isLambda) {
initScope(isLambda, false);
public synchronized InterpreterContext prepareForInterpretation() {
initScope(false);

checkRelinearization();

Expand All @@ -654,14 +645,7 @@ public synchronized InterpreterContext prepareForInterpretation(boolean isLambda
/* SSS FIXME: Do we need to synchronize on this? Cache this info in a scope field? */
/** Run any necessary passes to get the IR ready for compilation */
public synchronized List<BasicBlock> prepareForCompilation() {
// For lambdas, we need to add a global ensure block to catch
// uncaught breaks and throw a LocalJumpError.
//
// Since we dont re-JIT a previously JIT-ted closure,
// mark all closures lambdas always. But, check if there are
// other smarts available to us and eliminate adding
// this code to every closure there is.
initScope(this instanceof IRClosure, true);
initScope(true);

runCompilerPasses(getManager().getJITPasses(this));

Expand Down Expand Up @@ -1068,13 +1052,6 @@ public InterpreterContext getInstrsForInterpretation() {
return interpreterContext;
}

public InterpreterContext getInstrsForInterpretation(boolean isLambda) {
if (interpreterContext == null) {
prepareForInterpretation(isLambda);
}
return interpreterContext;
}

public void resetLinearizationData() {
linearizedBBList = null;
relinearizeCFG = false;
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/IRScriptBody.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public boolean isScriptScope() {
}

public IRubyObject interpret(ThreadContext context, IRubyObject self) {
prepareForInterpretation(false);
prepareForInterpretation();

String name = "(root)";
if (IRRuntimeHelpers.isDebug()) {
Expand Down
21 changes: 13 additions & 8 deletions core/src/main/java/org/jruby/ir/dataflow/analyses/StoreLocalVarPlacementProblem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,21 @@ public void addStores(Map<Operand, Operand> varRenameMap) {

// Allocate global-ensure block, if necessary
if ((mightRequireGlobalEnsureBlock == true) && !dirtyVars.isEmpty()) {
Variable exc = cfgScope.createTemporaryVariable();
BasicBlock geb = new BasicBlock(cfg, new Label("_GLOBAL_ENSURE_BLOCK", 0));

ListIterator instrs = geb.getInstrs().listIterator();
ListIterator<Instr> instrs;
BasicBlock geb = cfg.getGlobalEnsureBB();
if (geb == null) {
Variable exc = cfgScope.createTemporaryVariable();
geb = new BasicBlock(cfg, Label.GLOBAL_ENSURE_BLOCK_LABEL);
geb.addInstr(new ReceiveJRubyExceptionInstr(exc)); // JRuby implementation exception handling
geb.addInstr(new ThrowExceptionInstr(exc));
cfg.addGlobalEnsureBB(geb);
}

instrs.add(new ReceiveJRubyExceptionInstr(exc)); // JRuby implementation exception handling
instrs = geb.getInstrs().listIterator(geb.getInstrs().size());
Instr i = instrs.previous();
// Assumption: Last instr should always be a control-transfer instruction
assert i.getOperation().transfersControl(): "Last instruction of GEB in scope: " + getScope() + " is " + i + ", not a control-xfer instruction";
addClosureExitStoreLocalVars(instrs, dirtyVars, varRenameMap);
instrs.add(new ThrowExceptionInstr(exc));

cfg.addGlobalEnsureBB(geb);
}
}
}
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/instructions/BacktickInstr.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,6 @@ public void visit(IRVisitor visitor) {

@Override
public String toString() {
return result + "`" + (pieces == null ? "[]" : pieces) + "`";
return result + " = `" + (pieces == null ? "[]" : pieces) + "`";
}
}
6 changes: 3 additions & 3 deletions core/src/main/java/org/jruby/ir/interpreter/Interpreter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public static IRubyObject interpretCommonEval(Ruby runtime, String file, int lin
StaticScope ss = rootNode.getStaticScope();
IRScope containingIRScope = getEvalContainerScope(ss, evalType);
IREvalScript evalScript = IRBuilder.createIRBuilder(runtime, runtime.getIRManager()).buildEvalRoot(ss, containingIRScope, file, lineNumber, rootNode, evalType);
evalScript.prepareForInterpretation(false);
evalScript.prepareForInterpretation();
ThreadContext context = runtime.getCurrentContext();

IRubyObject rv = null;
Expand Down Expand Up @@ -121,7 +121,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.prepareForInterpretation(false);
b.prepareForInterpretation();
Block blk = (Block)(new WrappedIRClosure(b.getSelf(), b)).retrieve(context, self, currScope, context.getCurrentScope(), temp);
blk.yield(context, null);
}
Expand Down Expand Up @@ -525,7 +525,7 @@ private static DynamicScope getNewDynScope(ThreadContext context, IRScope scope,

private static IRubyObject interpret(ThreadContext context, IRubyObject self,
IRScope scope, Visibility visibility, RubyModule implClass, String name, IRubyObject[] args, Block block, Block.Type blockType) {
InterpreterContext interpreterContext = scope.getInstrsForInterpretation(blockType == Block.Type.LAMBDA);
InterpreterContext interpreterContext = scope.getInstrsForInterpretation();
Instr[] instrs = interpreterContext.getInstructions();
int numTempVars = interpreterContext.getTemporaryVariablecount();
Object[] temp = numTempVars > 0 ? new Object[numTempVars] : null;
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/org/jruby/ir/operands/Label.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// there is exactly one label object with the same label string?
public class Label extends Operand {
public static final Label UNRESCUED_REGION_LABEL = new Label("UNRESCUED_REGION", 0);
public static final Label GLOBAL_ENSURE_BLOCK_LABEL = new Label("_GLOBAL_ENSURE_BLOCK_", 0);

public final String prefix;
public final int id;
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ public Object execute(IRScope scope, Object... data) {
boolean bindingHasEscaped = scope.bindingHasEscaped();

CFG cfg = scope.cfg();
BasicBlock geb = cfg.getGlobalEnsureBB();

if (slvpp != null && bindingHasEscaped) {
scopeHasLocalVarStores = slvpp.scopeHasLocalVarStores();
Expand Down Expand Up @@ -123,9 +122,10 @@ public Object execute(IRScope scope, Object... data) {
if (requireBinding) entryBB.addInstr(new PushBindingInstr());

// Allocate GEB if necessary for popping
BasicBlock geb = cfg.getGlobalEnsureBB();
if (geb == null) {
Variable exc = scope.createTemporaryVariable();
geb = new BasicBlock(cfg, new Label("_GLOBAL_ENSURE_BLOCK", 0));
geb = new BasicBlock(cfg, Label.GLOBAL_ENSURE_BLOCK_LABEL);
geb.addInstr(new ReceiveJRubyExceptionInstr(exc)); // JRuby Implementation exception handling
geb.addInstr(new ThrowExceptionInstr(exc));
cfg.addGlobalEnsureBB(geb);
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/jruby/ir/representations/CFG.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,9 @@ private BasicBlock buildExitBasicBlock(Stack<ExceptionRegion> nestedExceptionReg
private BasicBlock createBB(Label label, Stack<ExceptionRegion> nestedExceptionRegions) {
BasicBlock basicBlock = new BasicBlock(this, label);
addBasicBlock(basicBlock);
if (label.equals(Label.GLOBAL_ENSURE_BLOCK_LABEL)) {
globalEnsureBB = basicBlock;
}

if (!nestedExceptionRegions.empty()) nestedExceptionRegions.peek().addBB(basicBlock);

Expand Down
10 changes: 6 additions & 4 deletions core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,13 @@ public static IRubyObject handleNonlocalReturn(StaticScope scope, DynamicScope d
} else {
IRReturnJump rj = (IRReturnJump)rjExc;

// - If we are in a lambda or if we are in the method scope we are supposed to return from, stop propagating
// If we are in a lambda or if we are in the method scope we are supposed to return from, stop propagating.
if (inNonMethodBodyLambda(scope, blockType) || (rj.methodToReturnFrom == dynScope)) {
if (isDebug()) System.out.println("---> Non-local Return reached target in scope: " + dynScope);
if (isDebug()) System.out.println("---> Non-local Return reached target in scope: " + dynScope + " matching dynscope? " + (rj.methodToReturnFrom == dynScope));
return (IRubyObject) rj.returnValue;
}

// - If not, Just pass it along!
// If not, Just pass it along!
throw rj;
}
}
Expand Down Expand Up @@ -147,7 +147,9 @@ public static IRubyObject handleBreakAndReturnsInLambdas(ThreadContext context,
if ((exc instanceof IRBreakJump) && inNonMethodBodyLambda(scope, blockType)) {
// We just unwound all the way up because of a non-local break
throw IRException.BREAK_LocalJumpError.getException(context.getRuntime());
} else if (exc instanceof IRReturnJump) {
} else if (exc instanceof IRReturnJump && (blockType == null || inLambda(blockType))) {
// Ignore non-local return processing in non-lambda blocks.
// Methods have a null blocktype
return handleNonlocalReturn(scope, dynScope, exc, blockType);
} else {
// Propagate
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public void emitScope(IRScope scope, String name, Signature signature) {
if (prepare) {
bbs = scope.prepareForCompilation();
} else {
scope.prepareForInterpretation(false);
scope.prepareForInterpretation();
bbs = scope.buildLinearization();
}
Map <BasicBlock, Label> exceptionTable = scope.buildJVMExceptionTable();
Expand Down
Loading

0 comments on commit 4a5d818

Please sign in to comment.