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

cgen: remove unused code generated for unwrapping temp var from callexpr (detect unused return value from CallExpr) #22769

Merged
merged 19 commits into from
Nov 7, 2024

Conversation

felipensp
Copy link
Member

@felipensp felipensp commented Nov 5, 2024

This PR adds a flag on ast.CallExpr to state about its return value is used. Using such information we can handle better the temporary storage for returning values or not.

image

Huly®: V_0.6-21216

@felipensp
Copy link
Member Author

felipensp commented Nov 5, 2024

About 40 unused option tmp var data access removed from .c file of cmd/v

+  (*(int*)_t8.data);
+  (*(int*)_t9.data);
+  (*(int*)_t10.data);
+  (*(int*)_t11.data);
+  (*(int*)_t12.data);
+  (*(int*)_t13.data);
+  (*(int*)_t15.data);
+  (*(int*)_t16.data);

@spytheman
Copy link
Member

Why are they marked with +, instead of -?

@felipensp
Copy link
Member Author

Why are they marked with +, instead of -?

My diff -uwp new old command with misordered file arg. 👍🏻

@spytheman
Copy link
Member

oh, ok

@felipensp felipensp changed the title cgen: remove unused code generated for unwrapping temp var from callexpr cgen: remove unused code generated for unwrapping temp var from callexpr (detect unused return value from CallExpr) Nov 5, 2024
@felipensp felipensp marked this pull request as ready for review November 5, 2024 22:17
@felipensp felipensp marked this pull request as draft November 6, 2024 00:51
@felipensp felipensp marked this pull request as ready for review November 6, 2024 16:55
vlib/v2/types/checker.v Outdated Show resolved Hide resolved
@spytheman
Copy link
Member

Is there a performance impact?

@StunxFS
Copy link
Contributor

StunxFS commented Nov 6, 2024

I think this could be handled if the checker warned for every expression whose value had not been used, and recommended using _ = instead, so the codegen would generate (void)<expr>.

@felipensp
Copy link
Member Author

I think this could be handled if the checker warned for every expression whose value had not been used, and recommended using _ = instead, so the codegen would generate (void)<expr>.

Yes, with this new flag is_return_used we can provide new compiler features/optimizations.

@felipensp
Copy link
Member Author

Is there a performance impact?

I haven't noticed perf impacts.

@StunxFS
Copy link
Contributor

StunxFS commented Nov 6, 2024

I think this could be handled if the checker warned for every expression whose value had not been used, and recommended using _ = instead, so the codegen would generate (void)<expr>.

Yes, with this new flag is_return_used we can provide new compiler features/optimizations.

@felipensp What I have in mind does not require .is_return_used, it would be enough for the checker, when checking a list of statements, to check if the last statement is of a type other than void, if so (and if it is not the last statement of an if or match block) that the checker warned about.

@felipensp
Copy link
Member Author

I'm working on it to set the flag about value usage in parse-time... removing the checker sets right now.

@spytheman
Copy link
Member

spytheman commented Nov 6, 2024

I'm working on it to set the flag about value usage in parse-time... removing the checker sets right now.

You can not do it reliably in the parser, since it does not have the complete type info.

For example, you can have a call to a function declared in another .v file in the same module, that may not be parsed yet, and you will not know if the current call expression returns something or not.

In the checker, since all the .v files are already parsed, the table is filled with all the symbols declared in all of them, and you so you know for sure what every call expression refers to.

@felipensp
Copy link
Member Author

felipensp commented Nov 6, 2024

I'm working on it to set the flag about value usage in parse-time... removing the checker sets right now.

You can not do it reliably in the parser, since it does not have the complete type info.

For example, you can have a call to a function declared in another .v file in the same module, that may not be parsed yet, and you will not know if the current call expression returns something or not.

In the checker, since all the .v files are already parsed, the table is filled with all the symbols declared in all of them, and you so you know for sure what every call expression refers to.

I think we're talking about different thing. I mean about knowing wether the value is being used or not in the code, not about expr types checking. This is my objective here.

@StunxFS
Copy link
Contributor

StunxFS commented Nov 6, 2024

I'm working on it to set the flag about value usage in parse-time... removing the checker sets right now.

You can not do it reliably in the parser, since it does not have the complete type info.
For example, you can have a call to a function declared in another .v file in the same module, that may not be parsed yet, and you will not know if the current call expression returns something or not.
In the checker, since all the .v files are already parsed, the table is filled with all the symbols declared in all of them, and you so you know for sure what every call expression refers to.

I think we're talking about different thing. I mean about knowing wether the value is being used or not in the code, not about expr types checking. This is my objective here.

And how do you know if the type of the returned value is other than void?

@felipensp
Copy link
Member Author

felipensp commented Nov 6, 2024

I'm working on it to set the flag about value usage in parse-time... removing the checker sets right now.

You can not do it reliably in the parser, since it does not have the complete type info.
For example, you can have a call to a function declared in another .v file in the same module, that may not be parsed yet, and you will not know if the current call expression returns something or not.
In the checker, since all the .v files are already parsed, the table is filled with all the symbols declared in all of them, and you so you know for sure what every call expression refers to.

I think we're talking about different thing. I mean about knowing wether the value is being used or not in the code, not about expr types checking. This is my objective here.

And how do you know if the type of the returned value is other than void?

My goal is not check the returned type but if the possible return value is used or not.

@spytheman
Copy link
Member

If you have just f() in some place, you do not know if f() returned something, and that is being ignored, or if did not return anything at all, without knowing the full declaration of fn f() and the return type.

@spytheman
Copy link
Member

spytheman commented Nov 7, 2024

I guess for the purpose of code generation, just having a plain f(), instead of a := f() or g(f()), is enough to know that the potential returned value is ignored, but that is not the case for using that information in more complex ways in the checker.

(except for x := if cond { f() } else { g() }, or match expressions, or default values in or blocks)

@spytheman
Copy link
Member

Anyway, I did some performance checks, and there is not indeed a perceivable performance difference between the change here and on master, and the generated code is cleaner here.

Excellent work.

@spytheman spytheman merged commit dab25ca into vlang:master Nov 7, 2024
71 checks passed
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.

3 participants