-
Notifications
You must be signed in to change notification settings - Fork 951
Add a mechanism to disable interp passes #2003
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
Conversation
c816302
to
e4cf47f
Compare
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.
In general this looks good, except for the two things below.
That said, I'm wondering how these debug options can best be separated from other options. Maybe by prefixing all these options with -internal
for example, like -internal-interp-pass
and -internal-printir
. Regular users shouldn't use these options. Or even make them a bit harder to use using something like -build-debug="interp-all,dumpssa"
. But that's for a separate issue/PR, not for this one.
CI is failing, probably because the func (c *Config) InterpProgram() bool {
switch c.Options.InterpPass {
case "package", "none":
return false
}
return true
} |
Thank you for the feedback @aykevl. I've update the code. |
Any feedback on this now @aykevl? |
e017ca0
to
d06ccda
Compare
On the subject of configuring |
So I still don't think this change is correct in general. Some packages absolutely require the interp pass to run for correct functioning. Although, maybe this is only true for the runtime package and not for other packages. One thing that would be correct is to disable the whole program interp pass (
I guess? But I think that would only be for debugging. If it runs more than a few seconds I'd argue that there must be a bug somewhere (even if it's just a performance bug). So, not sure about this but not against it either. |
The idea behind this PR was to disable the interp passes for debugging, and not to generate fully functional code. Would you be more confortable if we prefix the parameter with |
Thinking a bit more about this, perhaps one piece of information missing here is the fact that this was meant to be used together with |
This would be handy when debugging code generation issues and should
not be used in normal circumstances.
Discussed with @aykevl