-
Notifications
You must be signed in to change notification settings - Fork 21
Jb/stack checks #55
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
base: master
Are you sure you want to change the base?
Jb/stack checks #55
Conversation
compiler/stz-reg-alloc.stanza
Outdated
| ;===================== Driver =============================== | ||
| ;============================================================ | ||
|
|
||
| val SMALL-SMAP-SIZE = 16 ;; SIZE OF SMAP WHICH CAN BE CALLED WITHOUT CHECKING STACK-SIZE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a
;====================================
;========== PARAMETERS ===============
;====================================
section at the top of the file and put this definition there.
Please follow code comment conventions. Comments start with a single semi-colon are capitalized and then lower-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| val amb-branch = label-table[amb(e)] | ||
| E $ asm-MethodDispatch(multi(e), num-header-args(e), no-branch, amb-branch) | ||
| (e) : fatal("Not yet implemented: %_" % [e]) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is too fragile. Let's introduce a new instruction called:
SectionMarker :
name: Symbol
And have the preamble be surrounded by a pair of SectionMarker instructions. Then strip away everything in that section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| val return-lbl = unique-id(stubs) | ||
| E $ SetL(R0, LM(return-lbl)) | ||
| E $ SetL(R1, INT(size(stackmap) + 8)) | ||
| E $ SetL(R1, INT(size(stackmap) + (8 + SMALL-SMAP-SIZE))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be the right logic, but before I can merge it I need to refresh my head about the details of the stack layout. It's tricky, and if we get it wrong, it manifests as random crashes every few weeks, so it's important we get it right on the first try.
To do that, I need to know what is the stack layout, what is enforced by this instruction, why that +8 is there, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the +8 is yours. i just added SMALL-SMAP-SIZE.
compiler/stz-compiler-main.stanza
Outdated
| switch(situation) : | ||
| `optimized-asm : | ||
| val packages = Vector<VMPackage|StdPkg>() | ||
| val opt-timer = MillisecondTimer("Opt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cleanup before merge.
CuppoJava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a useful but tricky request to merge. Will need time to review.
remove stack check if function is small and doesn't have any non-tail-calls.