Is compileSubroutine correct?

[fishboy1887]

I have struggled a lot with the subroutine part of generating VM code. After around ~40 hours of struggling I decided to re-write it as well as re-read chapter 11 and I have produced the following for compileSubroutine:

void CompilationEngine::compileSubroutine() {
    // ('constructor'|'function'|'method') ('void'|type) subroutineName '('paramaterList')' subroutineBody
    symbolTable.reset();

    // p.foo(), allign p object w this
    // push p
    // pop pointer 0

    // ('constructor'|'function'|'method')
    std::string subroutineCat = tokenzier.identifier();
    tokenizer.advance();

    // (void|type)
    std::string subroutineType;
    if (tokenizer.tokenType() == JackTokenizer::TokenElements::KEYWORD)
        subroutineType = tokenizer.getCurrentToken();
    else
        subroutineType = tokenizer.identifier();
    tokenizer.advance();

    tokenizer.advance();    // subroutineName
    std::string subroutineName = tokenizer.identifier();
    std::string fullSubroutineName = currentClass + "." + tokenzier.identifier();

    if (subroutineCat == "method")
        symbolTableSubroutine.define(currentClass + "." + tokenizer.identifier(), "this", "argument");

    // '('paramaterList')'
    tokenizer.advance();    // (
    compileParamaterList();
    tokenizer.advance();    // )

    vmWriter.writeFunction(fullSubroutineName, symbolTableSubroutine.varCount("local"));

    // Allign this with base address on which object was called upon
    if (subroutineCat == "method") {
        vmWriter.writePush("argument", 0);
        vmWriter.writePop("pointer", 0);
    } else if (subroutineCat == "constructor") {
        vmWriter.writePush("constant", symbolTableSubroutine.varCount("field"));
        vmWriter.writeCall("Memory.alloc", 1);
    }

    // subroutineBody '{' varDec* statements '}'
    compileSubroutineBody();

    if (subroutineCat == "constructor") {
        vmWriter.writePush("pointer", 0);
        vmWriter.writeReturn();
    }

    if (subroutineType == "void") {
        vmWriter.writePush("constant", 0);
        vmWriter.writeReturn();
    }
}

Would anyone be able to tell me if this is correct, incorrect or missing anything?

Thanks

[pm100]

not going to read this code (yet) but  its more important to show us what it generates, thats easy to tell.

And does what you generate work?

So show what you generate for a couple of sample jack functions

[fishboy1887]

function Main.main 0
push constant 8001
push constant 16
push constant 1
neg
call Main.fillMemory 3
pop temp 0
push constant 8000
call Memory.peek 1
pop local 0
push local 0
call Main.convert 1
pop temp 0
return
push constant 0
return
function Main.convert 0
push constant 1
neg
pop local 2
label WHILE_EXP0
push local 2
not
if-goto WHILE_END0
push local 1
push constant 1
add
pop local 1
push local 0
call Main.nextMask 1
pop local 0
push local 1
not
not
if-goto IF_FALSE0
goto IF_TRUE0
label IF_FALSE0
label IF_TRUE0
goto WHILE_EXP0
goto WHILE_END0
push argument 0
not
not
if-goto IF_FALSE1
goto IF_TRUE1
label IF_FALSE1
label IF_TRUE1
push constant 0
return


This is the VM code generated when running ConvertToBin/Main.vm

The main issue is how the subroutines are handled. The first error to appear is that there is a call to Main.fillmemory but the compiler never compiles Main.fillmemory. I also think there may be some issue with the loops since the VM code does not look right to me.

[pm100]

without seeing the rest of your compiler code its impossible to say why you stopped compiling the class when only half way through. The code you show is compiling a function, need to see the code that calls it

[pm100]

you also didnt finish compiling convert, where are the calls to poke?

[fishboy1887]

https://github.com/blacknand/nand2tetris/blob/compiler_II/10_jack_compiler/Compiler/CompilationEngine.cpp

This is the full code. That is the VM code, it is cut short and I am currently stepping through to find out why.

[pm100]

most likely cause is that the compileStatements (or one of the things it calls) function is exiting early and the current token is placed wrong so the main while driver loop looking for a function start exits early

[fishboy1887]

I found that I had not removed the XML formatted <, >, & and " tokens so I removed them and the VM program finished. I think the main issue here now is that the VM lines such as function Main.nextMask 0 do not have the correct number of local variables

function Main.main 0
push constant 8001
push constant 16
push constant 1
neg
call Main.fillMemory 3
pop temp 0
push constant 8000
call Memory.peek 1
pop local 0
push local 0
call Main.convert 1
pop temp 0
return
push constant 0
return
function Main.convert 0
push constant 1
neg
pop local 2
label WHILE_EXP0
push local 2
not
if-goto WHILE_END0
push local 1
push constant 1
add
pop local 1
push local 0
call Main.nextMask 1
pop local 0
push local 1
push constant 16
gt
not
not
if-goto IF_FALSE0
push argument 0
push local 0
and
push constant 0
eq
not
not
if-goto IF_FALSE1
push constant 8000
push local 1
add
push constant 1
call Memory.poke 2
pop temp 0
goto IF_END1
label IF_FALSE1
push constant 8000
push local 1
add
push constant 0
call Memory.poke 2
pop temp 0
label IF_END1
goto IF_END0
label IF_FALSE0
push constant 0
pop local 2
label IF_END0
goto WHILE_EXP0
label WHILE_END0
return
push constant 0
return
function Main.nextMask 0
push argument 0
push constant 0
eq
not
if-goto IF_FALSE2
push constant 1
return
goto IF_END2
label IF_FALSE2
push argument 0
push constant 2
call Math.multiply 2
return
label IF_END2
function Main.fillMemory 0
label WHILE_EXP1
push argument 1
push constant 0
gt
not
if-goto WHILE_END1
push argument 0
push argument 2
call Memory.poke 2
pop temp 0
push argument 1
push constant 1
sub
pop argument 1
push argument 0
push constant 1
add
pop argument 0
goto WHILE_EXP1
label WHILE_END1
return
push constant 0
return

[fishboy1887]

I seem to be completely stuck. I have been doing this for a full week and just cannot get past it at all, currently I am stuck trying to figure out how to find the number of local variables, before generating any of the functions VM code. I found cadet1620's post on it and tried it where you write the function VM command just after all local variables have been declared but it did not work.

This is my output for ConvertToBin/Main.jack:

function Main.main 1
push constant 8001
push constant 16
push constant 1
neg
call Main.fillMemory 3
pop temp 0
push constant 8000
call Memory.peek 1
pop local 0
push local 0
call Main.convert 1
pop temp 0
return
push constant 0
return
function Main.convert 2
function Main.convert 3
push constant 1
neg
pop local 2
label WHILE_EXP0
push local 2
not
if-goto WHILE_END0
push local 1
push constant 1
add
pop local 1
push local 0
call Main.nextMask 1
pop local 0
push local 1
push constant 16
gt
not
not
if-goto IF_FALSE0
push argument 0
push local 0
and
push constant 0
eq
not
not
if-goto IF_FALSE1
push constant 8000
push local 1
add
push constant 1
call Memory.poke 2
pop temp 0
goto IF_END1
label IF_FALSE1
push constant 8000
push local 1
add
push constant 0
call Memory.poke 2
pop temp 0
label IF_END1
goto IF_END0
label IF_FALSE0
push constant 0
pop local 2
label IF_END0
goto WHILE_EXP0
label WHILE_END0
return
push constant 0
return
push argument 0
push constant 0
eq
not
if-goto IF_FALSE2
push constant 1
return
goto IF_END2
label IF_FALSE2
push argument 0
push constant 2
call Math.multiply 2
return
label IF_END2
return
label WHILE_EXP1
push argument 1
push constant 0
gt
not
if-goto WHILE_END1
push argument 0
push argument 2
call Memory.poke 2
pop temp 0
push argument 1
push constant 1
sub
pop argument 1
push argument 0
push constant 1
add
pop argument 0
goto WHILE_EXP1
label WHILE_END1
return
push constant 0
return

[dolomiti7]

I presume that your parser generating XML code in project 10 worked correctly, so we can exclude the possibility of a flawed parser. A good way of narrowing down the problem, is to compare your VM code with the code generated by the official Jack compiler.

I haven't looked into the code in detail, so there might be other problems. There are a couple of obvious things, however:

Your return code is not correct:

call Main.convert 1
pop temp 0
return // there is no return value on the stack here
push constant 0
return // double return; unreachable code

Also you are now emitting function statements twice:

function Main.convert 2
function Main.convert 3

Looking into your repo:
1. You are not supposed to issue return statements yourself. The Jack language requires that returns are explicitly defined in the source code. If you look at the sample code of Pong in project 11, you will see that every function, method or constructor have their return statement in the code. Constructors end with return this, functions and methods end with either return <expr> or a simple "return;" in case of void functions. Therefore you shouldn't issue your own return statements in compileSubroutine. This should all be handled by your compileReturn. (Please note that in other languages, there is no explicit return for void functions or constructors required, so for such languages you would need to track if the source code contains already a return statement.)
2. The treatment of return without expression should be done inside compileReturn. If there is no expression, you emit a zero constant prior to the return statement.
3. I'm not sure how your code ended up with 2 consecutive function statements in your VM code, but that is obviously wrong

Remark: Keeping track of the return type can help to issue syntax errors, for example if you have a void function that ends with return <expr>, or a non-void function that doesn't return a value. This is not required for a correct compiler though. You can (or should) assume that the source code is correct. Only after you have a correctly working compiler, you may consider to add in further functionality - keep it simple.

[pm100]

with regards to counting function locals. The jack language is very compiler friendly, all the local variables come before the statements of a function so you do this

for each local var
    count++
    update local symbol table

now emit "function ... <count>"

now process the statements.

You do not emit the vm function statement till you hit the first statement of the function body

[fishboy1887]

I have implemented fixes for the errors you have identifier.

function Square.new 0
push constant 0
call Memory.alloc 1
push pointer 0
push argument 0
pop field 0
push argument 1
pop field 1
push argument 2
pop field 2
push pointer 0
call Square.draw 1
pop temp 0
push pointer 0
return

After compiling Square I still cannot get it right. As you can see in the snippet of the first 15 VM commands, there are the commands "pop field 0" which is not correct, it should be pop this 0. I am not sure where I am going wrong though, I stepped through it and both of these Jack variables:

   field Square square; // the square of this game
   field int direction; // the square's current direction:

are correctly added into the class level symbol table as a variable of type square and a kind of field. Then inside of CompileLet the class level symbol table is searched, and the result is popped onto the stack which seems correct to me so I am not sure why it is meant to do pop this 0. I will add in the link of my repo for reference: https://github.com/blacknand/nand2tetris/tree/compiler_II/10_jack_compiler/Compiler

[fishboy1887]

Thanks, your solution worked

[pm100]

your repo does not include VMRtiter.cpp so I have to guess.

I guess that you do 'push <type> index' where 'type' is the value you use to disinguish different symbol types. For the fields of a class you use the word 'field', you need to map that to 'this' before you write  the push

[fishboy1887]

Yes you were right. I have regenerated Square/Square.jack and have produced the following VM code which still is not 100% correct but is getting there.

function Square.new 0
push constant 0
call Memory.alloc 1
push pointer 0
push argument 0
pop this 0
push argument 1
pop this 1
push argument 2
pop this 2
push pointer 0
call Square.draw 1
pop temp 0
push pointer 0
return
function Square.dispose 0
push argument 0
pop pointer 0
push pointer 0
call Memory.deAlloc 1
pop temp 0
push constant 0
return
function Square.draw 0
push argument 0
pop pointer 0
push constant 1
neg
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 0
push this 2
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
push constant 0
return
function Square.erase 0
push argument 0
pop pointer 0
push constant 0
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 0
push this 2
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
push constant 0
return
function Square.incSize 0
push argument 0
pop pointer 0
push this 1
push this 2
add
push constant 254
lt
push this 0
push this 2
add
push constant 510
lt
and
if-goto IF_TRUE0
goto IF_FALSE0
label IF_TRUE0
push pointer 0
call Square.erase 1
pop temp 0
push this 2
push constant 2
add
pop this 2
push pointer 0
call Square.draw 1
pop temp 0
goto IF_END0
label IF_FALSE0
label IF_END0
push constant 0
return
function Square.decSize 0
push argument 0
pop pointer 0
push this 2
push constant 2
gt
if-goto IF_TRUE0
goto IF_FALSE0
label IF_TRUE0
push pointer 0
call Square.erase 1
pop temp 0
push this 2
push constant 2
sub
pop this 2
push pointer 0
call Square.draw 1
pop temp 0
goto IF_END0
label IF_FALSE0
label IF_END0
push constant 0
return
function Square.moveUp 0
push argument 0
pop pointer 0
push this 1
push constant 1
gt
if-goto IF_TRUE0
goto IF_FALSE0
label IF_TRUE0
push constant 0
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 2
add
push constant 1
sub
push this 0
push this 2
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
push this 1
push constant 2
sub
pop this 1
push constant 1
neg
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 0
push this 2
add
push this 1
push constant 1
add
call Screen.drawRectangle 4
pop temp 0
goto IF_END0
label IF_FALSE0
label IF_END0
push constant 0
return
function Square.moveDown 0
push argument 0
pop pointer 0
push this 1
push this 2
add
push constant 254
lt
if-goto IF_TRUE0
goto IF_FALSE0
label IF_TRUE0
push constant 0
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 0
push this 2
add
push this 1
push constant 1
add
call Screen.drawRectangle 4
pop temp 0
push this 1
push constant 2
add
pop this 1
push constant 1
neg
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 2
add
push constant 1
sub
push this 0
push this 2
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
goto IF_END0
label IF_FALSE0
label IF_END0
push constant 0
return
function Square.moveLeft 0
push argument 0
pop pointer 0
push this 0
push constant 1
gt
if-goto IF_TRUE0
goto IF_FALSE0
label IF_TRUE0
push constant 0
call Screen.setColor 1
pop temp 0
push this 0
push this 2
add
push constant 1
sub
push this 1
push this 0
push this 2
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
push this 0
push constant 2
sub
pop this 0
push constant 1
neg
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 0
push constant 1
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
goto IF_END0
label IF_FALSE0
label IF_END0
push constant 0
return
function Square.moveRight 0
push argument 0
pop pointer 0
push this 0
push this 2
add
push constant 510
lt
if-goto IF_TRUE0
goto IF_FALSE0
label IF_TRUE0
push constant 0
call Screen.setColor 1
pop temp 0
push this 0
push this 1
push this 0
push constant 1
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
push this 0
push constant 2
add
pop this 0
push constant 1
neg
call Screen.setColor 1
pop temp 0
push this 0
push this 2
add
push constant 1
sub
push this 1
push this 0
push this 2
add
push this 1
push this 2
add
call Screen.drawRectangle 4
pop temp 0
goto IF_END0
label IF_FALSE0
label IF_END0
push constant 0
return


I will continue debugging in the morning, I have now spent all day on it (7 days total on just translating the XML tags to VM commands!) but I also wanted to mention that VMWriter.cpp is in the repo.

[dolomiti7]

Your memory allocation for fields is not correct.
Square new should reserve 3 words in memory for its fields. Your code reserves 0...

function Square.new 0
push constant 0 // ???
call Memory.alloc 1

[fishboy1887]

I have just passed all tests. I want to thank both you and pm100 for helping me. Think has been the hardest project so far and I would not have done it without both of you helping.