Move call form of block_given? to its own instr

The optimized block_given? still needs to be treated as a call, which would pollute the flags for defined?(yield) unnecessarily. This patch moves it to its own instr.
headius · Mar 27, 2024 · 35068f0 · 35068f0
1 parent c623b34
commit 35068f0
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 46 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRVisitor.java b/core/src/main/java/org/jruby/ir/IRVisitor.java
@@ -37,6 +37,7 @@ private void error(Object object) {
     public void AttrAssignInstr(AttrAssignInstr attrassigninstr) { error(attrassigninstr); }
     public void BFalseInstr(BFalseInstr bfalseinstr) { error(bfalseinstr); }
     public void BlockGivenInstr(BlockGivenInstr blockgiveninstr) { error(blockgiveninstr); }
+    public void BlockGivenCallInstr(BlockGivenCallInstr blockgivencallinstr) { error(blockgivencallinstr); }
     public void BIntInstr(BIntInstr bIntInstr) { error(bIntInstr); }
     public void BNEInstr(BNEInstr bneinstr) { error(bneinstr); }
     public void BNilInstr(BNilInstr bnilinstr) { error(bnilinstr); }

diff --git a/core/src/main/java/org/jruby/ir/Operation.java b/core/src/main/java/org/jruby/ir/Operation.java
@@ -83,6 +83,7 @@ public enum Operation {
     CALL_1OB(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
     CALL_0O(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
     NORESULT_CALL_1O(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
+    BLOCK_GIVEN_CALL(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
 
     /** Ruby operators: should all these be calls? Implementing instrs don't inherit from CallBase.java */
     EQQ(OpFlags.f_has_side_effect | OpFlags.f_can_raise_exception), // a === call used in when

diff --git a/core/src/main/java/org/jruby/ir/builder/IRBuilderAST.java b/core/src/main/java/org/jruby/ir/builder/IRBuilderAST.java
@@ -2042,7 +2042,7 @@ public Operand buildFCall(Variable result, FCallNode node) {
             case "iterator?":
                 if (node.getArgsNode() == null
                         && node.getIterNode() == null) {
-                    addInstr(new BlockGivenInstr(result, getYieldClosureVariable(), callName));
+                    addInstr(new BlockGivenCallInstr(result, getYieldClosureVariable(), callName));
                     return result;
                 }
         }

diff --git a/core/src/main/java/org/jruby/ir/instructions/BlockGivenCallInstr.java b/core/src/main/java/org/jruby/ir/instructions/BlockGivenCallInstr.java
@@ -0,0 +1,67 @@
+package org.jruby.ir.instructions;
+
+import org.jruby.ir.IRVisitor;
+import org.jruby.ir.Operation;
+import org.jruby.ir.operands.Operand;
+import org.jruby.ir.operands.Variable;
+import org.jruby.ir.persistence.IRReaderDecoder;
+import org.jruby.ir.persistence.IRWriterEncoder;
+import org.jruby.ir.runtime.IRRuntimeHelpers;
+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.runtime.callsite.FunctionalCachingCallSite;
+
+import java.util.Objects;
+
+/**
+ * A call to block_given? which can be optimized like defined?(yield) or a regular call.
+ */
+public class BlockGivenCallInstr extends OneOperandResultBaseInstr implements FixedArityInstr {
+    private final String callName;
+    private final FunctionalCachingCallSite blockGivenSite;
+
+    public BlockGivenCallInstr(Variable result, Operand block, String callName) {
+        super(Operation.BLOCK_GIVEN_CALL, Objects.requireNonNull(result, "BlockGivenCallInstr result is null"), block);
+
+        this.callName = Objects.requireNonNull(callName, "BlockGivenCallInstr callName is null");
+        this.blockGivenSite = new FunctionalCachingCallSite(callName);
+    }
+
+    public Operand getBlockArg() {
+        return getOperand1();
+    }
+
+    public String getCallName() {
+        return callName;
+    }
+
+    @Override
+    public Instr clone(CloneInfo ii) {
+        return new BlockGivenCallInstr(ii.getRenamedVariable(result), getBlockArg().cloneForInlining(ii), callName);
+    }
+
+    public static BlockGivenCallInstr decode(IRReaderDecoder d) {
+        return new BlockGivenCallInstr(d.decodeVariable(), d.decodeOperand(), d.decodeString());
+    }
+
+    @Override
+    public void encode(IRWriterEncoder e) {
+        super.encode(e);
+        e.encode(callName);
+    }
+
+    @Override
+    public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
+        Object blk = getBlockArg().retrieve(context, self, currScope, currDynScope, temp);
+
+        return IRRuntimeHelpers.blockGivenOrCall(context, self, blockGivenSite, blk);
+    }
+
+    @Override
+    public void visit(IRVisitor visitor) {
+        visitor.BlockGivenCallInstr(this);
+    }
+}
diff --git a/core/src/main/java/org/jruby/ir/instructions/BlockGivenInstr.java b/core/src/main/java/org/jruby/ir/instructions/BlockGivenInstr.java
@@ -5,72 +5,41 @@
 import org.jruby.ir.operands.Operand;
 import org.jruby.ir.operands.Variable;
 import org.jruby.ir.persistence.IRReaderDecoder;
-import org.jruby.ir.persistence.IRWriterEncoder;
 import org.jruby.ir.runtime.IRRuntimeHelpers;
 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.runtime.callsite.FunctionalCachingCallSite;
+
+import java.util.Objects;
 
 /**
  * Represents a defined?(yield) check, which works like a call to block_given? without
  * requiring special access to the caller's frame.
  */
 public class BlockGivenInstr extends OneOperandResultBaseInstr implements FixedArityInstr {
-    private final String callName;
-    private final FunctionalCachingCallSite blockGivenSite;
-
     public BlockGivenInstr(Variable result, Operand block) {
-        this(result, block, null);
-    }
-
-    public BlockGivenInstr(Variable result, Operand block, String callName) {
-        super(Operation.BLOCK_GIVEN, result, block);
-
-        assert result != null: "BlockGivenInstr result is null";
-
-        this.callName = callName;
-
-        if (callName == null) {
-            blockGivenSite = null;
-        } else {
-            blockGivenSite = new FunctionalCachingCallSite(callName);
-        }
+        super(Operation.BLOCK_GIVEN, Objects.requireNonNull(result, "BlockGivenCallInstr result is null"), block);
     }
 
     public Operand getBlockArg() {
         return getOperand1();
     }
 
-    public String getCallName() {
-        return callName;
-    }
-
     @Override
     public Instr clone(CloneInfo ii) {
-        return new BlockGivenInstr(ii.getRenamedVariable(result), getBlockArg().cloneForInlining(ii), callName);
+        return new BlockGivenInstr(ii.getRenamedVariable(result), getBlockArg().cloneForInlining(ii));
     }
 
     public static BlockGivenInstr decode(IRReaderDecoder d) {
-        return new BlockGivenInstr(d.decodeVariable(), d.decodeOperand(), d.decodeString());
-    }
-
-    @Override
-    public void encode(IRWriterEncoder e) {
-        super.encode(e);
-        e.encode(callName);
+        return new BlockGivenInstr(d.decodeVariable(), d.decodeOperand());
     }
 
     @Override
     public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
         Object blk = getBlockArg().retrieve(context, self, currScope, currDynScope, temp);
 
-        if (callName != null) {
-            return IRRuntimeHelpers.blockGivenOrCall(context, self, blockGivenSite, blk);
-        }
-
         return IRRuntimeHelpers.isBlockGiven(context, blk);
     }
 

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -816,21 +816,20 @@ public void BFalseInstr(BFalseInstr bFalseInstr) {
 
     @Override
     public void BlockGivenInstr(BlockGivenInstr blockGivenInstr) {
-        String callName = blockGivenInstr.getCallName();
-
-        if (callName != null) {
-            IRBytecodeAdapter m = jvmMethod();
-            m.getInvocationCompiler().invokeBlockGiven(callName, file);
-            handleCallResult(m, blockGivenInstr.getResult());
-            return;
-        }
-
         jvmMethod().loadContext();
         visit(blockGivenInstr.getBlockArg());
         jvmMethod().invokeIRHelper("isBlockGiven", sig(RubyBoolean.class, ThreadContext.class, Object.class));
         jvmStoreLocal(blockGivenInstr.getResult());
     }
 
+    @Override
+    public void BlockGivenCallInstr(BlockGivenCallInstr blockGivenCallInstr) {
+        String callName = blockGivenCallInstr.getCallName();
+        IRBytecodeAdapter m = jvmMethod();
+        m.getInvocationCompiler().invokeBlockGiven(callName, file);
+        handleCallResult(m, blockGivenCallInstr.getResult());
+    }
+
     @Override
     public void BIntInstr(BIntInstr bIntInstr) {
         visit(bIntInstr.getArg1());