Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: jruby/jruby
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 2ed16bce439d^
Choose a base ref
...
head repository: jruby/jruby
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: e236c543b72f
Choose a head ref
  • 2 commits
  • 9 files changed
  • 1 contributor

Commits on Aug 10, 2017

  1. Copy the full SHA
    2ed16bc View commit details

Commits on Aug 11, 2017

  1. Remove BEQ.

    BEQ appeared to only get used by case/when statements where a
    when has a literal array as a first element. This meant that for
    when with a literal array, we were calling #== instead of #===.
    
    I could not reproduce this behavior in MRI, and the following
    code shows that we are doing something wrong.
    
    ```ruby
    class Array
      def ===(other)
        p :===
        self == other
      end
    end
    
    case x
    when []
    end
    ```
    
    MRI prints out :=== while JRuby does not.
    
    This logic apears to be from when we used to process whens with
    multiple arguments differently. Today, those cases get built as
    separate conditions, and I believe this removed code is no longer
    valid.
    headius committed Aug 11, 2017
    Copy the full SHA
    e236c54 View commit details
129 changes: 71 additions & 58 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
Original file line number Diff line number Diff line change
@@ -827,7 +827,7 @@ public Operand buildAnd(final AndNode andNode) {
Label l = getNewLabel();
Operand v1 = build(andNode.getFirstNode());
Variable ret = getValueInTemporaryVariable(v1);
addInstr(BEQInstr.create(v1, manager.getFalse(), l));
addInstr(createBranch(v1, manager.getFalse(), l));
Operand v2 = build(andNode.getSecondNode());
addInstr(new CopyInstr(ret, v2));
addInstr(new LabelInstr(l));
@@ -1134,42 +1134,12 @@ public Operand buildCase(CaseNode caseNode) {

Variable eqqResult = createTemporaryVariable();
labels.add(bodyLabel);
Operand v1, v2;
if (whenNode.getExpressionNodes() instanceof ListNode
// DNode produces a proper result, so we don't want the special ListNode handling below
// FIXME: This is obviously gross, and we need a better way to filter out non-expression ListNode here
// See GH #2423
&& !(whenNode.getExpressionNodes() instanceof DNode)) {
// Note about refactoring:
// - BEQInstr has a quick implementation when the second operand is a boolean literal
// If it can be fixed to do this even on the first operand, we can switch around
// v1 and v2 in the UndefinedValue scenario and DRY out this code.
// - Even with this asymmetric implementation of BEQInstr, you might be tempted to
// switch around v1 and v2 in the else case. But, that is equivalent to this Ruby code change:
// (v1 == value) instead of (value == v1)
// It seems that they should be identical, but the first one is v1.==(value) and the second one is
// value.==(v1). This is just fine *if* the Ruby programmer has implemented an algebraically
// symmetric "==" method on those objects. If not, then, the results might be unexpected where the
// code (intentionally or otherwise) relies on this asymmetry of "==". While it could be argued
// that this a Ruby code bug, we will just try to preserve the order of the == check as it appears
// in the Ruby code.
if (value == UndefinedValue.UNDEFINED) {
v1 = build(whenNode.getExpressionNodes());
v2 = manager.getTrue();
} else {
v1 = value;
v2 = build(whenNode.getExpressionNodes());
}
} else {
Operand expression = buildWithOrder(whenNode.getExpressionNodes(), whenNode.containsVariableAssignment());
Node exprNodes = whenNode.getExpressionNodes();
boolean needsSplat = exprNodes instanceof ArgsPushNode || exprNodes instanceof SplatNode || exprNodes instanceof ArgsCatNode;
Operand expression = buildWithOrder(whenNode.getExpressionNodes(), whenNode.containsVariableAssignment());
Node exprNodes = whenNode.getExpressionNodes();
boolean needsSplat = exprNodes instanceof ArgsPushNode || exprNodes instanceof SplatNode || exprNodes instanceof ArgsCatNode;

addInstr(new EQQInstr(eqqResult, expression, value, needsSplat));
v1 = eqqResult;
v2 = manager.getTrue();
}
addInstr(BEQInstr.create(v1, v2, bodyLabel));
addInstr(new EQQInstr(eqqResult, expression, value, needsSplat));
addInstr(createBranch(eqqResult, manager.getTrue(), bodyLabel));

// SSS FIXME: This doesn't preserve original order of when clauses. We could consider
// preserving the order (or maybe not, since we would have to sort the constants first
@@ -1315,7 +1285,7 @@ public int compare(Map.Entry<Integer, Label> o1, Map.Entry<Integer, Label> o2) {
v1 = eqqResult;
v2 = manager.getTrue();
}
addInstr(BEQInstr.create(v1, v2, bodyLabel));
addInstr(createBranch(v1, v2, bodyLabel));

// SSS FIXME: This doesn't preserve original order of when clauses. We could consider
// preserving the order (or maybe not, since we would have to sort the constants first
@@ -1620,7 +1590,7 @@ public Operand buildGetDefinition(Node node) {
for (Node elt: array.children()) {
Operand result = buildGetDefinition(elt);

addInstr(BEQInstr.create(result, manager.getNil(), undefLabel));
addInstr(createBranch(result, manager.getNil(), undefLabel));
}

addInstr(new CopyInstr(tmpVar, new FrozenString(DefinedMessage.EXPRESSION.getText())));
@@ -1698,7 +1668,7 @@ public Operand buildGetDefinition(Node node) {
}
)
);
addInstr(BEQInstr.create(tmpVar, manager.getNil(), undefLabel));
addInstr(createBranch(tmpVar, manager.getNil(), undefLabel));
Operand superDefnVal = buildGetArgumentDefinition(((SuperNode) node).getArgsNode(), DefinedMessage.SUPER.getText());
return buildDefnCheckIfThenPaths(undefLabel, superDefnVal);
}
@@ -1778,7 +1748,7 @@ public Operand run() {
Label done = getNewLabel();
Variable result = createTemporaryVariable();
Operand test = buildGetDefinition(((Colon2Node) colon).getLeftNode());
addInstr(BEQInstr.create(test, manager.getNil(), bad));
addInstr(createBranch(test, manager.getNil(), bad));
Operand lhs = build(((Colon2Node) colon).getLeftNode());
addInstr(
new RuntimeHelperCall(
@@ -1833,7 +1803,7 @@ public Operand run() {
}
)
);
addInstr(BEQInstr.create(tmpVar, manager.getNil(), undefLabel));
addInstr(createBranch(tmpVar, manager.getNil(), undefLabel));
Operand argsCheckDefn = buildGetArgumentDefinition(((FCallNode) node).getArgsNode(), "method");
return buildDefnCheckIfThenPaths(undefLabel, argsCheckDefn);
}
@@ -1845,7 +1815,7 @@ public Operand run() {
public Operand run() {
final Label undefLabel = getNewLabel();
Operand receiverDefn = buildGetDefinition(callNode.getReceiverNode());
addInstr(BEQInstr.create(receiverDefn, manager.getNil(), undefLabel));
addInstr(createBranch(receiverDefn, manager.getNil(), undefLabel));
Variable tmpVar = createTemporaryVariable();
addInstr(
new RuntimeHelperCall(
@@ -1878,7 +1848,7 @@ public Operand run() {
public Operand run() {
final Label undefLabel = getNewLabel();
Operand receiverDefn = buildGetDefinition(attrAssign.getReceiverNode());
addInstr(BEQInstr.create(receiverDefn, manager.getNil(), undefLabel));
addInstr(createBranch(receiverDefn, manager.getNil(), undefLabel));
/* --------------------------------------------------------------------------
* This basically combines checks from CALLNODE and FCALLNODE
*
@@ -1907,7 +1877,7 @@ public Operand run() {
}
)
);
addInstr(BEQInstr.create(tmpVar, manager.getNil(), undefLabel));
addInstr(createBranch(tmpVar, manager.getNil(), undefLabel));
Operand argsCheckDefn = buildGetArgumentDefinition(attrAssign.getArgsNode(), "assignment");
return buildDefnCheckIfThenPaths(undefLabel, argsCheckDefn);
}
@@ -1939,7 +1909,7 @@ protected Variable buildDefnCheckIfThenPaths(Label undefLabel, Operand defVal) {
protected Variable buildDefinitionCheck(ResultInstr definedInstr, String definedReturnValue) {
Label undefLabel = getNewLabel();
addInstr((Instr) definedInstr);
addInstr(BEQInstr.create(definedInstr.getResult(), manager.getFalse(), undefLabel));
addInstr(createBranch(definedInstr.getResult(), manager.getFalse(), undefLabel));
return buildDefnCheckIfThenPaths(undefLabel, new FrozenString(definedReturnValue));
}

@@ -1958,7 +1928,7 @@ public Operand buildGetArgumentDefinition(final Node node, String type) {
break;
} else if (!def.hasKnownValue()) { // Optimization!
failPathReqd = true;
addInstr(BEQInstr.create(def, manager.getNil(), failLabel));
addInstr(createBranch(def, manager.getNil(), failLabel));
}
}
} else {
@@ -1967,7 +1937,7 @@ public Operand buildGetArgumentDefinition(final Node node, String type) {
rv = manager.getNil();
} else if (!def.hasKnownValue()) { // Optimization!
failPathReqd = true;
addInstr(BEQInstr.create(def, manager.getNil(), failLabel));
addInstr(createBranch(def, manager.getNil(), failLabel));
}
}

@@ -2805,7 +2775,7 @@ public Operand buildFlip(FlipNode flipNode) {
addInstr(new LabelInstr(s2Label));

// For exclusive ranges/flips, we dont evaluate s2's condition if s1's condition was satisfied
if (flipNode.isExclusive()) addInstr(BEQInstr.create(returnVal, manager.getTrue(), doneLabel));
if (flipNode.isExclusive()) addInstr(createBranch(returnVal, manager.getTrue(), doneLabel));

// Are we in state 2?
addInstr(BNEInstr.create(doneLabel, flipState, s2));
@@ -2934,7 +2904,7 @@ public Operand buildIf(Variable result, final IfNode ifNode) {
Label falseLabel = getNewLabel();
Label doneLabel = getNewLabel();
Operand thenResult;
addInstr(BEQInstr.create(build(actualCondition), manager.getFalse(), falseLabel));
addInstr(createBranch(build(actualCondition), manager.getFalse(), falseLabel));

boolean thenNull = false;
boolean elseNull = false;
@@ -3211,7 +3181,7 @@ public Operand buildOpAsgn(OpAsgnNode opAsgnNode) {
String opName = opAsgnNode.getOperatorName();
if (opName.equals("||") || opName.equals("&&")) {
l = getNewLabel();
addInstr(BEQInstr.create(readerValue, opName.equals("||") ? manager.getTrue() : manager.getFalse(), l));
addInstr(createBranch(readerValue, opName.equals("||") ? manager.getTrue() : manager.getFalse(), l));

// compute value and set it
Operand v2 = build(opAsgnNode.getValueNode());
@@ -3293,7 +3263,7 @@ public Operand buildOpAsgnAnd(OpAsgnAndNode andNode) {
Label l = getNewLabel();
Operand v1 = build(andNode.getFirstNode());
Variable result = getValueInTemporaryVariable(v1);
addInstr(BEQInstr.create(v1, manager.getFalse(), l));
addInstr(createBranch(v1, manager.getFalse(), l));
Operand v2 = build(andNode.getSecondNode()); // This does the assignment!
addInstr(new CopyInstr(result, v2));
addInstr(new LabelInstr(l));
@@ -3318,15 +3288,15 @@ public Operand buildOpAsgnOr(final OpAsgnOrNode orNode) {
l2 = getNewLabel();
v1 = buildGetDefinition(orNode.getFirstNode());
addInstr(new CopyInstr(flag, v1));
addInstr(BEQInstr.create(flag, manager.getNil(), l2)); // if v1 is undefined, go to v2's computation
addInstr(createBranch(flag, manager.getNil(), l2)); // if v1 is undefined, go to v2's computation
}
v1 = build(orNode.getFirstNode()); // build of 'x'
addInstr(new CopyInstr(flag, v1));
Variable result = getValueInTemporaryVariable(v1);
if (needsDefnCheck) {
addInstr(new LabelInstr(l2));
}
addInstr(BEQInstr.create(flag, manager.getTrue(), l1)); // if v1 is defined and true, we are done!
addInstr(createBranch(flag, manager.getTrue(), l1)); // if v1 is defined and true, we are done!
Operand v2 = build(orNode.getSecondNode()); // This is an AST node that sets x = y, so nothing special to do here.
addInstr(new CopyInstr(result, v2));
addInstr(new LabelInstr(l1));
@@ -3354,7 +3324,7 @@ private Operand buildOpElementAsgnWith(OpElementAsgnNode opElementAsgnNode, Bool
Variable elt = createTemporaryVariable();
Operand[] argList = setupCallArgs(opElementAsgnNode.getArgsNode());
addInstr(CallInstr.create(scope, callType, elt, "[]", array, argList, null));
addInstr(BEQInstr.create(elt, truthy, endLabel));
addInstr(createBranch(elt, truthy, endLabel));
Operand value = build(opElementAsgnNode.getValueNode());

argList = addArg(argList, value);
@@ -3408,7 +3378,7 @@ public Operand buildOr(final OrNode orNode) {
Label endOfExprLabel = getNewLabel();
Operand left = build(orNode.getFirstNode());
Variable result = getValueInTemporaryVariable(left);
addInstr(BEQInstr.create(left, manager.getTrue(), endOfExprLabel));
addInstr(createBranch(left, manager.getTrue(), endOfExprLabel));
Operand right = build(orNode.getSecondNode());
addInstr(new CopyInstr(result, right));
addInstr(new LabelInstr(endOfExprLabel));
@@ -3603,7 +3573,7 @@ private Operand buildRescueInternal(RescueNode rescueNode, EnsureBlockInfo ensur

private void outputExceptionCheck(Operand excType, Operand excObj, Label caughtLabel) {
Variable eqqResult = addResultInstr(new RescueEQQInstr(createTemporaryVariable(), excType, excObj));
addInstr(BEQInstr.create(eqqResult, manager.getTrue(), caughtLabel));
addInstr(createBranch(eqqResult, manager.getTrue(), caughtLabel));
}

private void buildRescueBodyInternal(RescueBodyNode rescueBodyNode, Variable rv, Variable exc, Label endLabel) {
@@ -3856,7 +3826,7 @@ private Operand buildConditionalLoop(Node conditionNode,
addInstr(new LabelInstr(loop.loopStartLabel));
if (isLoopHeadCondition) {
Operand cv = build(conditionNode);
addInstr(BEQInstr.create(cv, isWhile ? manager.getFalse() : manager.getTrue(), setupResultLabel));
addInstr(createBranch(cv, isWhile ? manager.getFalse() : manager.getTrue(), setupResultLabel));
}

// Redo jumps here
@@ -3874,7 +3844,7 @@ private Operand buildConditionalLoop(Node conditionNode,
addInstr(new JumpInstr(loop.loopStartLabel));
} else {
Operand cv = build(conditionNode);
addInstr(BEQInstr.create(cv, isWhile ? manager.getTrue() : manager.getFalse(), loop.iterStartLabel));
addInstr(createBranch(cv, isWhile ? manager.getTrue() : manager.getFalse(), loop.iterStartLabel));
}

// Loop result -- nil always
@@ -4152,4 +4122,47 @@ private static Operand[] getCallOperands(IRScope scope, List<Operand> callArgs,

return callArgs.toArray(new Operand[callArgs.size()]);
}

public static Instr createBranch(Operand v1, Operand v2, Label jmpTarget) {
if (v2 instanceof Boolean) {
Boolean lhs = (Boolean) v2;

if (lhs.isTrue()) {
if (v1 instanceof Boolean) {
if (((Boolean) v1).isTrue()) { // true == true -> just jump
return new JumpInstr(jmpTarget);
} else { // false == true (this will never jump)
return NopInstr.NOP;
}
} else {
return new BTrueInstr(jmpTarget, v1);
}
} else if (lhs.isFalse()) {
if (v1 instanceof Boolean) {
if (((Boolean) v1).isFalse()) { // false == false -> just jump
return new JumpInstr(jmpTarget);
} else { // true == false (this will never jump)
return NopInstr.NOP;
}
} else {
return new BFalseInstr(jmpTarget, v1);
}
}
} else if (v2 instanceof Nil) {
if (v1 instanceof Nil) { // nil == nil -> just jump
return new JumpInstr(jmpTarget);
} else {
return new BNilInstr(jmpTarget, v1);
}
}
if (v2 == UndefinedValue.UNDEFINED) {
if (v1 == UndefinedValue.UNDEFINED) {
return new JumpInstr(jmpTarget);
} else {
return new BUndefInstr(jmpTarget, v1);
}
}

throw new RuntimeException("BUG: no BEQ");
}
}
1 change: 0 additions & 1 deletion core/src/main/java/org/jruby/ir/IRVisitor.java
Original file line number Diff line number Diff line change
@@ -34,7 +34,6 @@ private void error(Object object) {
public void AttrAssignInstr(AttrAssignInstr attrassigninstr) { error(attrassigninstr); }
public void ArrayDerefInstr(ArrayDerefInstr arrayderefinstr) { error(arrayderefinstr); }
public void BacktickInstr(BacktickInstr instr) { error(instr); }
public void BEQInstr(BEQInstr beqinstr) { error(beqinstr); }
public void BFalseInstr(BFalseInstr bfalseinstr) { error(bfalseinstr); }
public void BlockGivenInstr(BlockGivenInstr blockgiveninstr) { error(blockgiveninstr); }
public void BNEInstr(BNEInstr bneinstr) { error(bneinstr); }
1 change: 0 additions & 1 deletion core/src/main/java/org/jruby/ir/Operation.java
Original file line number Diff line number Diff line change
@@ -34,7 +34,6 @@ public enum Operation {

/** control-flow **/
JUMP(OpFlags.f_is_jump_or_branch),
BEQ(OpFlags.f_is_jump_or_branch),
BNE(OpFlags.f_is_jump_or_branch),
B_UNDEF(OpFlags.f_is_jump_or_branch),
B_NIL(OpFlags.f_is_jump_or_branch),
6 changes: 3 additions & 3 deletions core/src/main/java/org/jruby/ir/instructions/AliasInstr.java
Original file line number Diff line number Diff line change
@@ -45,9 +45,9 @@ public Instr clone(CloneInfo ii) {

@Override
public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
String newNameString = getNewName().retrieve(context, self, currScope, currDynScope, temp).toString();
String oldNameString = getOldName().retrieve(context, self, currScope, currDynScope, temp).toString();
IRRuntimeHelpers.defineAlias(context, self, currDynScope, newNameString, oldNameString);
IRubyObject newName = (IRubyObject) getNewName().retrieve(context, self, currScope, currDynScope, temp);
IRubyObject oldName = (IRubyObject) getOldName().retrieve(context, self, currScope, currDynScope, temp);
IRRuntimeHelpers.defineAlias(context, self, currDynScope, newName, oldName);
return null;
}

Loading