Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major Statement Restructuring with trade-offs #293

Closed
wants to merge 5 commits into from

Conversation

CAS-ual-TY
Copy link
Contributor

@CAS-ual-TY CAS-ual-TY commented Jul 1, 2021

Hello, it is me once again.
So, as mentioned in my last PR, I have restructured the compile_statement method. The result is not a direct improvement, to make that immediately clear, it is a trade-off.

Changes

Basically, assignments on dereferenced variables/expressions were completely split off from assignments on variables. This resulted in a bunch of code replication. (Dereferenced) variables had their address and offset "hardcoded" as parameters in the emit_store call, dereferenced expressions passed that on as parameter.
So I have changed the (dereferenced) variable address to be moved into a temporary and then used the same emit_store call for all of them, with the address now being stored and used in a temporary.
The code is more streamlined now because it first checks if there is an assignment, then moves the store-address into a temporary, then compiles the value and stores that in said address. Only the first step differs, the last 2 are always the same. This also includes type checks.

Trade-Off

Pro
The code for assignments is much more streamlined making array and struct implementation a bit more "direct" (you can immediately work on implementing the pointer offsets without having todo major restructuring yourself).

Contra
The generated code is 1 addi-instruction longer for each assignment to a dereferenced pointer variable (not dereferenced expression!) or to a variable in general.

Student Perspective - Why it makes it easier for students

We had a circle of helping each other (we called it "the circle of knowledge" because something someone found out would go around) when it came to the array and struct assignments (everyone had unique code at that point, so we didnt just copy it over; in fact, I helped one friend twice on the phone while driving from Munich to Salzburg :) ).
So I can say for myself that I have seen a bunch of different implementations and ideas, but one thing was forced in all of them: Variable assignments needed restructuring, because for arrays you inevitably need to be able to calculate with/using addresses. And for most students, this actually was the hardest part to do. It wasnt the actual offset calculation, it was the preperation for that.
This is why streamlining this part of the code and taking the trade-off might be beneficial for students. They can now clearly differentiate between the differences of dereferenced variables or expressions and variables in general and also see what they have in common. If you compare the way the method works out right now, compared to compile_factor: The later is a lot easier to study because it is a lot more streamlined.

Problems with this PR and Suggestions

  • In line 5050 (sorry, I dont know how to mark it directly on GitHub, like comment on it) I temporarily set the symbol back to an asterisk for the syntax message. This works but isnt very clean imo. The problem here is that we read an asterisk before a method call and want to show that the asterisk is misplaced in a call-statement, but we already read the next symbol at that point. I could make another method which is the same as syntax_error_unexpected, but it gets the unexpected symbol as parameter instead.
  • I suggest moving the assignments into a seperate method, and calling said method last in compile_statement, in an else-branch. This way there is no long-method and it would have the big advantage that the for-loop implementation would greatly benefit from this.

Final Words

This is a trade-off afterall and I do not have nearly as much expertise in this field as you, obviously, so this not being accepted wont be a big deal to me. I primarily do this as learning effect and simply because I really enjoy doing it. I tried my best to bring in a student's perspective and would love to hear what you say about the proposed changes.
Thanks!

Combined the way assignments on variables work. Basically, dereferenced pointers/expresions are now merging the end of their control flow with "normal" variable assignments.
Just in case this is accepted: Help out future students in understanding the code
@ckirsch
Copy link
Member

ckirsch commented Jul 2, 2021

@CAS-ual-TY There may be a way to improve the code in compile_statement but that requires more thought. Your approach just seems to shift complexity into a different corner. I suggest to maintain the parsing structure and instead focus on helper code. It may be worthwhile identifying the exact abstraction (procedure) needed for emitting store instructions for assignments into variables, similar to the code for emitting load instructions for loading variables and big integers. There is a duality that has not been captured yet properly.

@CAS-ual-TY
Copy link
Contributor Author

Unfortunately, I wont have time to look at this for the next couple of weeks. I would suggest putting this on hold or closing it. I will reevaluate (and maybe rework) the PR once I am back in Salzburg.

@ckirsch
Copy link
Member

ckirsch commented Jul 12, 2021

@CAS-ual-TY Sounds good. I just leave it open.

@ckirsch
Copy link
Member

ckirsch commented Mar 15, 2022

@CAS-ual-TY what should we do with this one?

@CAS-ual-TY
Copy link
Contributor Author

Lets talk about it on monday

@CAS-ual-TY
Copy link
Contributor Author

This can be closed. I will reopen this with fresh code once #314 is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants