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. Don't use strings for alias since we have symbols on hand.

    @headius
    headius committed Aug 10, 2017
    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
    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
View file
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
View file
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
View file
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
View file
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