Skip to content

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

Closed
wants to merge 4 commits into from

Conversation

fgsch
Copy link
Contributor

@fgsch fgsch commented Jul 16, 2021

This would be handy when debugging code generation issues and should
not be used in normal circumstances.
Discussed with @aykevl

@fgsch fgsch marked this pull request as draft July 16, 2021 09:47
@fgsch fgsch force-pushed the fgsch/interp-pass branch 2 times, most recently from c816302 to e4cf47f Compare July 18, 2021 20:49
Copy link
Member

@aykevl aykevl left a 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.

@aykevl
Copy link
Member

aykevl commented Jul 19, 2021

CI is failing, probably because the InterpPass option isn't set when running the tests in main_test.go. One simple way to fix this is to also allow the empty string to indicate "all" in compileopts/config.go, or by reversing the logic:

func (c *Config) InterpProgram() bool {
	switch c.Options.InterpPass {
	case "package", "none":
		return false
	}
	return true
}

@fgsch fgsch force-pushed the fgsch/interp-pass branch from e4cf47f to 7c8a37b Compare July 19, 2021 14:05
@fgsch
Copy link
Contributor Author

fgsch commented Jul 19, 2021

Thank you for the feedback @aykevl. I've update the code.

@fgsch fgsch marked this pull request as ready for review July 27, 2021 12:11
@deadprogram deadprogram requested a review from aykevl July 28, 2021 16:05
@fgsch fgsch force-pushed the fgsch/interp-pass branch from 7c8a37b to e017ca0 Compare July 29, 2021 15:17
@deadprogram
Copy link
Member

Any feedback on this now @aykevl?

This would be handy when debugging code generation issues and should
not be used in normal circumstances.
Discussed with @aykevl
@fgsch fgsch force-pushed the fgsch/interp-pass branch from e017ca0 to d06ccda Compare November 11, 2021 11:30
@dgryski
Copy link
Member

dgryski commented Nov 19, 2021

On the subject of configuring interp, being able to customize the timeout value (currently one minute) would also be useful.

@aykevl
Copy link
Member

aykevl commented Nov 19, 2021

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 (interp.Run), that one can I think be safely disabled. Maybe that already solves the biggest issues?

being able to customize the timeout value (currently one minute) would also be useful.

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.

@fgsch
Copy link
Contributor Author

fgsch commented Nov 25, 2021

The idea behind this PR was to disable the interp passes for debugging, and not to generate fully functional code.
In other words, this is purely for debugging and should not be used for anything else.

Would you be more confortable if we prefix the parameter with -internal as you suggested in #2003 (review) or should I go ahead and remove the package option, only leaving the program option?

@fgsch
Copy link
Contributor Author

fgsch commented Nov 26, 2021

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 -printir to capture the generated IR without the interp (package) pass.

@fgsch fgsch closed this Apr 28, 2022
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.

4 participants