Skip to content

Commit a662c18

Browse files
smarrsmarr
authored
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.
2 parents 4d7cb15 + 20d141b commit a662c18

File tree

12 files changed

+229
-23
lines changed

12 files changed

+229
-23
lines changed

‎src/compiler/MethodGenerationContext.cpp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -743,6 +743,31 @@ uint8_t MethodGenerationContext::GetInlinedLocalIdx(const Variable* var) const {
743743
ErrorExit(msg);
744744
}
745745

746+
const Variable* MethodGenerationContext::GetInlinedVariable(
747+
const Variable* oldVar) const {
748+
for (const Variable& v : locals) {
749+
if (v.IsSame(*oldVar)) {
750+
return &v;
751+
}
752+
}
753+
754+
for (const Variable& v : arguments) {
755+
if (v.IsSame(*oldVar)) {
756+
return &v;
757+
}
758+
}
759+
760+
if (outerGenc != nullptr) {
761+
return outerGenc->GetInlinedVariable(oldVar);
762+
}
763+
764+
char msg[100];
765+
const std::string* name = oldVar->GetName();
766+
(void)snprintf(msg, 100, "Failed to find inlined variable named %s.\n",
767+
name->c_str());
768+
ErrorExit(msg);
769+
}
770+
746771
void MethodGenerationContext::checkJumpOffset(size_t jumpOffset,
747772
uint8_t bytecode) {
748773
if (jumpOffset < 0 || jumpOffset > 0xFFFF) {

‎src/compiler/MethodGenerationContext.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ class MethodGenerationContext {
116116
void InlineAsLocals(vector<Variable>& vars);
117117

118118
uint8_t GetInlinedLocalIdx(const Variable* var) const;
119+
const Variable* GetInlinedVariable(const Variable* oldVar) const;
119120

120121
void EmitBackwardsJumpOffsetToTarget(size_t loopBeginIdx);
121122
void PatchJumpOffsetToPointToNextInstruction(size_t indexOfOffset);
@@ -183,4 +184,7 @@ class MethodGenerationContext {
183184
std::vector<BackJump> inlinedLoops;
184185

185186
bool isCurrentlyInliningABlock{false};
187+
188+
make_testable(public);
189+
vm_oop_t GetLiteral(size_t idx) { return literals.at(idx); }
186190
};

‎src/interpreter/bytecodes.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,11 @@ const char* Bytecode::bytecodeNames[] = {
172172
};
173173

174174
bool IsJumpBytecode(uint8_t bc) {
175-
static_assert(BC_JUMP < BC_JUMP2_BACKWARD);
176-
static_assert((BC_JUMP2_BACKWARD - BC_JUMP) == 21);
175+
static_assert(
176+
BC_JUMP < BC_JUMP2_BACKWARD,
177+
"make sure the nummeric value of jump bytecodes is as expected");
178+
static_assert((BC_JUMP2_BACKWARD - BC_JUMP) == 21,
179+
"we expect there to be 22 jump bytecodes");
177180

178181
return BC_JUMP <= bc && bc <= BC_JUMP2_BACKWARD;
179182
}

‎src/misc/debug.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include <string>
44

55
#include "../compiler/Disassembler.h"
6+
#include "../compiler/MethodGenerationContext.h"
67
#include "../vmobjects/ObjectFormats.h"
78
#include "../vmobjects/VMClass.h"
89
#include "../vmobjects/VMSymbol.h"
@@ -18,3 +19,7 @@ std::string DebugGetClassName(gc_oop_t obj) {
1819
void DebugDumpMethod(VMInvokable* method) {
1920
Disassembler::DumpMethod((VMMethod*)method, "", false);
2021
}
22+
23+
void DebugDumpMethod(MethodGenerationContext* mgenc) {
24+
Disassembler::DumpMethod(mgenc, "");
25+
}

‎src/misc/debug.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <cstdarg>
3030
#include <cstdio>
3131

32+
#include "../compiler/MethodGenerationContext.h"
3233
#include "../vmobjects/ObjectFormats.h"
3334

3435
#define FprintfPass(f, x) \
@@ -99,3 +100,4 @@ static inline void DebugTrace(const char* fmt, ...) {
99100
std::string DebugGetClassName(vm_oop_t /*obj*/);
100101
std::string DebugGetClassName(gc_oop_t /*obj*/);
101102
void DebugDumpMethod(VMInvokable* method);
103+
void DebugDumpMethod(MethodGenerationContext* mgenc);

‎src/unitTests/BytecodeGenerationTest.cpp‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -536,6 +536,128 @@ void BytecodeGenerationTest::testInliningOfToDo() {
536536
BC_RETURN_SELF});
537537
}
538538

539+
static std::vector<uint8_t> GetBytecodes(VMMethod* method) {
540+
std::vector<uint8_t> bcs(method->bcLength);
541+
542+
for (size_t i = 0; i < method->bcLength; i += 1) {
543+
bcs.at(i) = method->GetBytecode(i);
544+
}
545+
return bcs;
546+
}
547+
548+
void BytecodeGenerationTest::testToDoBlockBlockInlinedSelf() {
549+
auto bytecodes = methodToBytecode(R"""(
550+
test = (
551+
| l1 l2 |
552+
1 to: 2 do: [:a |
553+
l1 do: [:b |
554+
b ifTrue: [
555+
a.
556+
l2 := l2 + 1 ] ] ]
557+
) )""");
558+
check(bytecodes,
559+
{BC_PUSH_1, BC_PUSH_CONSTANT_0,
560+
BC_DUP_SECOND, // stack: Top[1, 2, 1]
561+
562+
BC(BC_JUMP_IF_GREATER, 15, 0), // consume only on jump
563+
BC_DUP,
564+
565+
BC_POP_LOCAL_2, // store the `a`
566+
BC_PUSH_LOCAL_0, // push the `l1` on the stack
567+
BC(BC_PUSH_BLOCK, 1), BC(BC_SEND, 2), // send #do:
568+
BC_POP,
569+
BC_INC, // increment top, the iteration counter
570+
571+
// jump back to the jump_if_greater bytecode
572+
BC(BC_JUMP_BACKWARD, 12, 0),
573+
574+
// jump_if_greater target
575+
BC_RETURN_SELF});
576+
577+
auto* block = (VMMethod*)_mgenc->GetLiteral(1);
578+
check(GetBytecodes(block),
579+
{BC_PUSH_ARG_1, BC(BC_JUMP_ON_FALSE_TOP_NIL, 15, 0),
580+
BC(BC_PUSH_LOCAL, 2, 1), // load the `a`
581+
BC_POP, BC(BC_PUSH_LOCAL, 1, 1), BC_INC, BC_DUP,
582+
BC(BC_POP_LOCAL, 1, 1), BC_RETURN_LOCAL},
583+
block);
584+
}
585+
586+
void BytecodeGenerationTest::testToDoWithMoreEmbeddedBlocksAndArgAccess() {
587+
auto bytecodes = methodToBytecode(R"""(
588+
transferEntries: oldStorage = (
589+
1 to: oldStorage length do: [:i |
590+
| current |
591+
current := oldStorage at: i.
592+
current notInlined: [
593+
oldStorage at: i put: nil.
594+
current next
595+
noInline: [ i. current. #foo ]
596+
noInline: [
597+
self splitBucket: oldStorage bucket: i head: current ] ] ]
598+
) )""");
599+
check(bytecodes,
600+
{BC_PUSH_1, BC_PUSH_ARG_1, BC(BC_SEND_1, 0),
601+
BC_DUP_SECOND, //~
602+
603+
BC(BC_JUMP_IF_GREATER, 20, 0), // consume only on jump
604+
BC_DUP,
605+
606+
BC_POP_LOCAL_0, // i
607+
BC_PUSH_ARG_1, // oldStorage
608+
BC_PUSH_LOCAL_0, // i
609+
BC(BC_SEND, 1), // #at:
610+
611+
BC_POP_LOCAL_1, // current
612+
BC_PUSH_LOCAL_1, // current
613+
BC(BC_PUSH_BLOCK, 2), // ~
614+
BC(BC_SEND, 3), // send #notInlined:
615+
BC_POP,
616+
BC_INC, // increment top, the iteration counter
617+
618+
// jump back to the jump_if_greater bytecode
619+
BC(BC_JUMP_BACKWARD, 17, 0),
620+
621+
// jump_if_greater target
622+
BC_RETURN_SELF});
623+
624+
auto* block = (VMMethod*)_mgenc->GetLiteral(2);
625+
check(GetBytecodes(block),
626+
{BC(BC_PUSH_ARGUMENT, 1, 1), // oldStorage
627+
BC(BC_PUSH_LOCAL, 0, 1), // i
628+
BC_PUSH_NIL, BC(BC_SEND, 0), // #at:put:
629+
BC_POP,
630+
631+
BC(BC_PUSH_LOCAL, 1, 1), // current
632+
BC(BC_SEND_1, 1), // #next
633+
BC(BC_PUSH_BLOCK, 2), //~
634+
BC(BC_PUSH_BLOCK, 3), //~
635+
BC(BC_SEND, 4), // #noInline:noInline:
636+
BC_RETURN_LOCAL},
637+
block);
638+
639+
// [ i. current. #foo ]
640+
auto* block2 = (VMMethod*)block->GetIndexableField(2);
641+
check(GetBytecodes(block2),
642+
{BC(BC_PUSH_LOCAL, 0, 2), // i
643+
BC_POP, //~
644+
BC(BC_PUSH_LOCAL, 1, 2), // current
645+
BC_POP, //~
646+
BC_PUSH_CONSTANT_0, BC_RETURN_LOCAL},
647+
block2);
648+
649+
// [ self splitBucket: oldStorage bucket: i head: current ]
650+
auto* block3 = (VMMethod*)block->GetIndexableField(3);
651+
check(GetBytecodes(block3),
652+
{BC(BC_PUSH_ARGUMENT, 0, 2), // self
653+
BC(BC_PUSH_ARGUMENT, 1, 2), // oldStorage
654+
BC(BC_PUSH_LOCAL, 0, 2), // i
655+
BC(BC_PUSH_LOCAL, 1, 2), // current
656+
BC(BC_SEND, 0), // #splitBucket:bucket:head:
657+
BC_RETURN_LOCAL},
658+
block3);
659+
}
660+
539661
void BytecodeGenerationTest::testIfArg() {
540662
ifArg("ifTrue:", BC_JUMP_ON_FALSE_TOP_NIL);
541663
ifArg("ifFalse:", BC_JUMP_ON_TRUE_TOP_NIL);

‎src/unitTests/BytecodeGenerationTest.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
class BytecodeGenerationTest : public TestWithParsing {
1111
// NOLINTNEXTLINE(misc-const-correctness)
1212
CPPUNIT_TEST_SUITE(BytecodeGenerationTest);
13+
1314
CPPUNIT_TEST(testEmptyMethodReturnsSelf);
1415
CPPUNIT_TEST(testPushConstant);
1516
CPPUNIT_TEST(testIfPushConstantSame);
@@ -50,7 +51,11 @@ class BytecodeGenerationTest : public TestWithParsing {
5051
CPPUNIT_TEST(testIfTrueIfFalseNlrArg2);
5152
CPPUNIT_TEST(testInliningOfOr);
5253
CPPUNIT_TEST(testInliningOfAnd);
54+
5355
CPPUNIT_TEST(testInliningOfToDo);
56+
CPPUNIT_TEST(testToDoBlockBlockInlinedSelf);
57+
CPPUNIT_TEST(testToDoWithMoreEmbeddedBlocksAndArgAccess);
58+
5459
CPPUNIT_TEST(testIfArg);
5560
CPPUNIT_TEST(testKeywordIfTrueArg);
5661
CPPUNIT_TEST(testIfReturnNonLocal);
@@ -147,6 +152,8 @@ class BytecodeGenerationTest : public TestWithParsing {
147152
void inliningOfAnd(std::string selector);
148153

149154
void testInliningOfToDo();
155+
void testToDoBlockBlockInlinedSelf();
156+
void testToDoWithMoreEmbeddedBlocksAndArgAccess();
150157

151158
static void testJumpQueuesOrdering();
152159

‎src/unitTests/TestWithParsing.cpp‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,15 @@ void TestWithParsing::dump(MethodGenerationContext* mgenc) {
2828
Disassembler::DumpMethod(mgenc, "");
2929
}
3030

31+
void TestWithParsing::dump(VMMethod* method) {
32+
if (method != nullptr) {
33+
Disassembler::DumpMethod(method, "");
34+
return;
35+
}
36+
37+
dump(static_cast<MethodGenerationContext*>(nullptr));
38+
}
39+
3140
void TestWithParsing::ensureCGenC() {
3241
if (_cgenc != nullptr) {
3342
return;
@@ -98,8 +107,7 @@ std::vector<uint8_t> TestWithParsing::blockToBytecode(const char* source,
98107
}
99108

100109
void TestWithParsing::check(std::vector<uint8_t> actual,
101-
std::vector<BC>
102-
expected) {
110+
std::vector<BC> expected, VMMethod* toDump) {
103111
size_t i = 0;
104112
size_t bci = 0;
105113
for (; bci < actual.size() && i < expected.size();) {
@@ -114,7 +122,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
114122
bci, Bytecode::GetBytecodeName(expectedBc.bytecode),
115123
Bytecode::GetBytecodeName(actualBc));
116124
if (expectedBc.bytecode != actualBc) {
117-
dump();
125+
dump(toDump);
118126
}
119127
CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.bytecode, actualBc);
120128

@@ -125,7 +133,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
125133
(size_t)bcLength);
126134

127135
if (expectedBc.size != bcLength) {
128-
dump();
136+
dump(toDump);
129137
}
130138
CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.size, (size_t)bcLength);
131139

@@ -135,7 +143,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
135143
i, Bytecode::GetBytecodeName(expectedBc.bytecode),
136144
expectedBc.arg1, actual.at(bci + 1));
137145
if (expectedBc.arg1 != actual.at(bci + 1)) {
138-
dump();
146+
dump(toDump);
139147
}
140148
CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.arg1,
141149
actual.at(bci + 1));
@@ -147,7 +155,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
147155
Bytecode::GetBytecodeName(expectedBc.bytecode),
148156
expectedBc.arg2, actual.at(bci + 2));
149157
if (expectedBc.arg2 != actual.at(bci + 2)) {
150-
dump();
158+
dump(toDump);
151159
}
152160
CPPUNIT_ASSERT_EQUAL_MESSAGE(msg, expectedBc.arg2,
153161
actual.at(bci + 2));
@@ -158,7 +166,7 @@ void TestWithParsing::check(std::vector<uint8_t> actual,
158166
bci += bcLength;
159167
}
160168
if (expected.size() != i || actual.size() != bci) {
161-
dump();
169+
dump(toDump);
162170
}
163171

164172
CPPUNIT_ASSERT_EQUAL_MESSAGE("All expected bytecodes covered",

‎src/unitTests/TestWithParsing.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ class TestWithParsing : public CPPUNIT_NS::TestCase {
5454
bool dumpBytecodes = false);
5555

5656
void dump(MethodGenerationContext* mgenc = nullptr);
57+
void dump(VMMethod* method = nullptr);
5758

58-
void check(std::vector<uint8_t> actual, std::vector<BC> expected);
59+
void check(std::vector<uint8_t> actual,
60+
std::vector<BC>
61+
expected,
62+
VMMethod* toDump = nullptr);
5963
};

‎src/vmobjects/VMFrame.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626
THE SOFTWARE.
2727
*/
2828

29+
#include <cstdint>
30+
#include <type_traits>
31+
2932
#include "../vmobjects/VMArray.h"
3033
#include "../vmobjects/VMMethod.h"
3134

0 commit comments

Comments
 (0)