Fix handling of inlined args and some bytecodes (#61)

The inlining logic did not yet account for the block argument of
`#to:do:` being turned into a local variable.

This PR uses the existing lexical scope and mgenc to look up the
matching new version of the previous argument.

It also adds handling for `DUP_SECOND` in `AdaptAfterOuterInlined` and
fixes the adapting of `INC_FIELD_PUSH` and `INC_FIELD` which do not have
explicit context arguments.

Author: smarr

src/compiler/MethodGenerationContext.cpp

@@ -743,6 +743,31 @@ uint8_t MethodGenerationContext::GetInlinedLocalIdx(const Variable* var) const {
     ErrorExit(msg);
 }
 
+const Variable* MethodGenerationContext::GetInlinedVariable(
+    const Variable* oldVar) const {
+    for (const Variable& v : locals) {
+        if (v.IsSame(*oldVar)) {
+            return &v;
+        }
+    }
+
+    for (const Variable& v : arguments) {
+        if (v.IsSame(*oldVar)) {
+            return &v;
+        }
+    }
+
+    if (outerGenc != nullptr) {
+        return outerGenc->GetInlinedVariable(oldVar);
+    }
+
+    char msg[100];
+    const std::string* name = oldVar->GetName();
+    (void)snprintf(msg, 100, "Failed to find inlined variable named %s.\n",
+                   name->c_str());
+    ErrorExit(msg);
+}
+
 void MethodGenerationContext::checkJumpOffset(size_t jumpOffset,
                                               uint8_t bytecode) {
     if (jumpOffset < 0 || jumpOffset > 0xFFFF) {

src/compiler/MethodGenerationContext.h

@@ -116,6 +116,7 @@ class MethodGenerationContext {
     void InlineAsLocals(vector<Variable>& vars);
 
     uint8_t GetInlinedLocalIdx(const Variable* var) const;
+    const Variable* GetInlinedVariable(const Variable* oldVar) const;
 
     void EmitBackwardsJumpOffsetToTarget(size_t loopBeginIdx);
     void PatchJumpOffsetToPointToNextInstruction(size_t indexOfOffset);
@@ -183,4 +184,7 @@ class MethodGenerationContext {
     std::vector<BackJump> inlinedLoops;
 
     bool isCurrentlyInliningABlock{false};
+
+    make_testable(public);
+    vm_oop_t GetLiteral(size_t idx) { return literals.at(idx); }
 };

src/interpreter/bytecodes.cpp

@@ -172,8 +172,11 @@ const char* Bytecode::bytecodeNames[] = {
 };
 
 bool IsJumpBytecode(uint8_t bc) {
-    static_assert(BC_JUMP < BC_JUMP2_BACKWARD);
-    static_assert((BC_JUMP2_BACKWARD - BC_JUMP) == 21);
+    static_assert(
+        BC_JUMP < BC_JUMP2_BACKWARD,
+        "make sure the nummeric value of jump bytecodes is as expected");
+    static_assert((BC_JUMP2_BACKWARD - BC_JUMP) == 21,
+                  "we expect there to be 22 jump bytecodes");
 
     return BC_JUMP <= bc && bc <= BC_JUMP2_BACKWARD;
 }

src/misc/debug.cpp

@@ -3,6 +3,7 @@
 #include <string>
 
 #include "../compiler/Disassembler.h"
+#include "../compiler/MethodGenerationContext.h"
 #include "../vmobjects/ObjectFormats.h"
 #include "../vmobjects/VMClass.h"
 #include "../vmobjects/VMSymbol.h"
@@ -18,3 +19,7 @@ std::string DebugGetClassName(gc_oop_t obj) {
 void DebugDumpMethod(VMInvokable* method) {
     Disassembler::DumpMethod((VMMethod*)method, "", false);
 }
+
+void DebugDumpMethod(MethodGenerationContext* mgenc) {
+    Disassembler::DumpMethod(mgenc, "");
+}

src/misc/debug.h

@@ -29,6 +29,7 @@
 #include <cstdarg>
 #include <cstdio>
 
+#include "../compiler/MethodGenerationContext.h"
 #include "../vmobjects/ObjectFormats.h"
 
 #define FprintfPass(f, x)         \
@@ -99,3 +100,4 @@ static inline void DebugTrace(const char* fmt, ...) {
 std::string DebugGetClassName(vm_oop_t /*obj*/);
 std::string DebugGetClassName(gc_oop_t /*obj*/);
 void DebugDumpMethod(VMInvokable* method);
+void DebugDumpMethod(MethodGenerationContext* mgenc);

src/unitTests/BytecodeGenerationTest.cpp

@@ -536,6 +536,128 @@ void BytecodeGenerationTest::testInliningOfToDo() {
            BC_RETURN_SELF});
 }
 
+static std::vector<uint8_t> GetBytecodes(VMMethod* method) {
+    std::vector<uint8_t> bcs(method->bcLength);
+
+    for (size_t i = 0; i < method->bcLength; i += 1) {
+        bcs.at(i) = method->GetBytecode(i);
+    }
+    return bcs;
+}
+
+void BytecodeGenerationTest::testToDoBlockBlockInlinedSelf() {
+    auto bytecodes = methodToBytecode(R"""(
+                                      test = (
+                                        | l1 l2 |
+                                        1 to: 2 do: [:a |
+                                            l1 do: [:b |
+                                            b ifTrue: [
+                                                a.
+                                                l2 := l2 + 1 ] ] ]
+                                      ) )""");
+    check(bytecodes,
+          {BC_PUSH_1, BC_PUSH_CONSTANT_0,
+           BC_DUP_SECOND,  // stack: Top[1, 2, 1]
+
+           BC(BC_JUMP_IF_GREATER, 15, 0),  // consume only on jump
+           BC_DUP,
+
+           BC_POP_LOCAL_2,                        // store the `a`
+           BC_PUSH_LOCAL_0,                       // push the `l1` on the stack
+           BC(BC_PUSH_BLOCK, 1), BC(BC_SEND, 2),  // send #do:
+           BC_POP,
+           BC_INC,  // increment top, the iteration counter
+
+           // jump back to the jump_if_greater bytecode
+           BC(BC_JUMP_BACKWARD, 12, 0),
+
+           // jump_if_greater target
+           BC_RETURN_SELF});
+
+    auto* block = (VMMethod*)_mgenc->GetLiteral(1);
+    check(GetBytecodes(block),
+          {BC_PUSH_ARG_1, BC(BC_JUMP_ON_FALSE_TOP_NIL, 15, 0),
+           BC(BC_PUSH_LOCAL, 2, 1),  // load the `a`
+           BC_POP, BC(BC_PUSH_LOCAL, 1, 1), BC_INC, BC_DUP,
+           BC(BC_POP_LOCAL, 1, 1), BC_RETURN_LOCAL},
+          block);
+}
+
+void BytecodeGenerationTest::testToDoWithMoreEmbeddedBlocksAndArgAccess() {
+    auto bytecodes = methodToBytecode(R"""(
+                                        transferEntries: oldStorage = (
+                                          1 to: oldStorage length do: [:i |
+                                            | current |
+                                            current := oldStorage at: i.
+                                            current notInlined: [
+                                              oldStorage at: i put: nil.
+                                              current next
+                                                noInline: [ i. current. #foo ]
+                                                noInline: [
+                                                  self splitBucket: oldStorage bucket: i head: current ] ] ]
+                                        ) )""");
+    check(bytecodes,
+          {BC_PUSH_1, BC_PUSH_ARG_1, BC(BC_SEND_1, 0),
+           BC_DUP_SECOND,  //~
+
+           BC(BC_JUMP_IF_GREATER, 20, 0),  // consume only on jump
+           BC_DUP,
+
+           BC_POP_LOCAL_0,   // i
+           BC_PUSH_ARG_1,    // oldStorage
+           BC_PUSH_LOCAL_0,  // i
+           BC(BC_SEND, 1),   // #at:
+
+           BC_POP_LOCAL_1,        // current
+           BC_PUSH_LOCAL_1,       // current
+           BC(BC_PUSH_BLOCK, 2),  // ~
+           BC(BC_SEND, 3),        // send #notInlined:
+           BC_POP,
+           BC_INC,  // increment top, the iteration counter
+
+           // jump back to the jump_if_greater bytecode
+           BC(BC_JUMP_BACKWARD, 17, 0),
+
+           // jump_if_greater target
+           BC_RETURN_SELF});
+
+    auto* block = (VMMethod*)_mgenc->GetLiteral(2);
+    check(GetBytecodes(block),
+          {BC(BC_PUSH_ARGUMENT, 1, 1),   // oldStorage
+           BC(BC_PUSH_LOCAL, 0, 1),      // i
+           BC_PUSH_NIL, BC(BC_SEND, 0),  // #at:put:
+           BC_POP,
+
+           BC(BC_PUSH_LOCAL, 1, 1),  // current
+           BC(BC_SEND_1, 1),         // #next
+           BC(BC_PUSH_BLOCK, 2),     //~
+           BC(BC_PUSH_BLOCK, 3),     //~
+           BC(BC_SEND, 4),           // #noInline:noInline:
+           BC_RETURN_LOCAL},
+          block);
+
+    // [ i. current. #foo ]
+    auto* block2 = (VMMethod*)block->GetIndexableField(2);
+    check(GetBytecodes(block2),
+          {BC(BC_PUSH_LOCAL, 0, 2),  // i
+           BC_POP,                   //~
+           BC(BC_PUSH_LOCAL, 1, 2),  // current
+           BC_POP,                   //~
+           BC_PUSH_CONSTANT_0, BC_RETURN_LOCAL},
+          block2);
+
+    // [ self splitBucket: oldStorage bucket: i head: current ]
+    auto* block3 = (VMMethod*)block->GetIndexableField(3);
+    check(GetBytecodes(block3),
+          {BC(BC_PUSH_ARGUMENT, 0, 2),  // self
+           BC(BC_PUSH_ARGUMENT, 1, 2),  // oldStorage
+           BC(BC_PUSH_LOCAL, 0, 2),     // i
+           BC(BC_PUSH_LOCAL, 1, 2),     // current
+           BC(BC_SEND, 0),              // #splitBucket:bucket:head:
+           BC_RETURN_LOCAL},
+          block3);
+}
+
 void BytecodeGenerationTest::testIfArg() {
     ifArg("ifTrue:", BC_JUMP_ON_FALSE_TOP_NIL);
     ifArg("ifFalse:", BC_JUMP_ON_TRUE_TOP_NIL);

src/unitTests/BytecodeGenerationTest.h

@@ -10,6 +10,7 @@
 class BytecodeGenerationTest : public TestWithParsing {
     // NOLINTNEXTLINE(misc-const-correctness)
     CPPUNIT_TEST_SUITE(BytecodeGenerationTest);
+
     CPPUNIT_TEST(testEmptyMethodReturnsSelf);
     CPPUNIT_TEST(testPushConstant);
     CPPUNIT_TEST(testIfPushConstantSame);
@@ -50,7 +51,11 @@ class BytecodeGenerationTest : public TestWithParsing {
     CPPUNIT_TEST(testIfTrueIfFalseNlrArg2);
     CPPUNIT_TEST(testInliningOfOr);
     CPPUNIT_TEST(testInliningOfAnd);
+
     CPPUNIT_TEST(testInliningOfToDo);
+    CPPUNIT_TEST(testToDoBlockBlockInlinedSelf);
+    CPPUNIT_TEST(testToDoWithMoreEmbeddedBlocksAndArgAccess);
+
     CPPUNIT_TEST(testIfArg);
     CPPUNIT_TEST(testKeywordIfTrueArg);
     CPPUNIT_TEST(testIfReturnNonLocal);
@@ -147,6 +152,8 @@ class BytecodeGenerationTest : public TestWithParsing {
     void inliningOfAnd(std::string selector);
 
     void testInliningOfToDo();
+    void testToDoBlockBlockInlinedSelf();
+    void testToDoWithMoreEmbeddedBlocksAndArgAccess();
 
     static void testJumpQueuesOrdering();
 

src/unitTests/TestWithParsing.cpp

@@ -28,6 +28,15 @@ void TestWithParsing::dump(MethodGenerationContext* mgenc) {
     Disassembler::DumpMethod(mgenc, "");
 }
 
+void TestWithParsing::dump(VMMethod* method) {
+    if (method != nullptr) {
+        Disassembler::DumpMethod(method, "");
+        return;
+    }
+
+    dump(static_cast<MethodGenerationContext*>(nullptr));
+}
+
 void TestWithParsing::ensureCGenC() {
     if (_cgenc != nullptr) {
         return;
@@ -98,8 +107,7 @@ std::vector<uint8_t> TestWithParsing::blockToBytecode(const char* source,
 }
 
 void TestWithParsing::check(std::vector<uint8_t> actual,
-                            std::vector<BC>
-                                expected) {
+                            std::vector<BC> expected, VMMethod* toDump) {
     size_t i = 0;
     size_t bci = 0;
     for (; bci < actual.size() && i < expected.size();) {
@@ -114,7 +122,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
                        bci, Bytecode::GetBytecodeName(expectedBc.bytecode),
                        Bytecode::GetBytecodeName(actualBc));
         if (expectedBc.bytecode != actualBc) {
-            dump();
+            dump(toDump);
         }
         CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.bytecode, actualBc);
 
@@ -125,7 +133,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
             (size_t)bcLength);
 
         if (expectedBc.size != bcLength) {
-            dump();
+            dump(toDump);
         }
         CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.size, (size_t)bcLength);
 
@@ -135,7 +143,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
                            i, Bytecode::GetBytecodeName(expectedBc.bytecode),
                            expectedBc.arg1, actual.at(bci + 1));
             if (expectedBc.arg1 != actual.at(bci + 1)) {
-                dump();
+                dump(toDump);
             }
             CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.arg1,
                                          actual.at(bci + 1));
@@ -147,7 +155,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
                     Bytecode::GetBytecodeName(expectedBc.bytecode),
                     expectedBc.arg2, actual.at(bci + 2));
                 if (expectedBc.arg2 != actual.at(bci + 2)) {
-                    dump();
+                    dump(toDump);
                 }
                 CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.arg2,
                                              actual.at(bci + 2));
@@ -158,7 +166,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
         bci += bcLength;
     }
     if (expected.size() != i || actual.size() != bci) {
-        dump();
+        dump(toDump);
     }
 
     CPPUNIT_ASSERT_EQUAL_MESSAGE("All expected bytecodes covered",

src/unitTests/TestWithParsing.h

@@ -54,6 +54,10 @@ class TestWithParsing : public CPPUNIT_NS::TestCase {
                                          bool dumpBytecodes = false);
 
     void dump(MethodGenerationContext* mgenc = nullptr);
+    void dump(VMMethod* method = nullptr);
 
-    void check(std::vector<uint8_t> actual, std::vector<BC> expected);
+    void check(std::vector<uint8_t> actual,
+               std::vector<BC>
+                   expected,
+               VMMethod* toDump = nullptr);
 };

src/vmobjects/VMFrame.h

@@ -26,6 +26,9 @@
  THE SOFTWARE.
  */
 
+#include <cstdint>
+#include <type_traits>
+
 #include "../vmobjects/VMArray.h"
 #include "../vmobjects/VMMethod.h"