Skip to content

Conversation

@dyniols
Copy link
Contributor

@dyniols dyniols commented Oct 2, 2024

The reason for this change is to avoid confusing when diagnostic is issued for functors that are defined but not used.

@dyniols dyniols marked this pull request as ready for review October 2, 2024 17:01
@dyniols dyniols requested a review from a team as a code owner October 2, 2024 17:01
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different from raising the warning on functions which are never called from a kernel? Should be be raising this warning on only those functions which are called from the kernel so you don't have random warnings generated for functions never even called?

@steffenlarsen
Copy link
Contributor

How is this different from raising the warning on functions which are never called from a kernel? Should be be raising this warning on only those functions which are called from the kernel so you don't have random warnings generated for functions never even called?

Free functions are not usable as kernels and these warnings should be issues when applying the attribute to anything that is not recognizably a kernel function. The problem with functors are that they could be defined in common headers, while only actually being used in some source files, which means for all other source files they would issue a warning, even if they are not being used in those source files.

Free functions are not usable as kernels ...

That is not completely true, but we may want to refine once we know how we apply these attributes to free functions in the extensions allowing these. Tag @AlexeySachkov for comment.

@elizabethandrews
Copy link
Contributor

elizabethandrews commented Oct 9, 2024

The problem with functors are that they could be defined in common headers, while only actually being used in some source files, which means for all other source files they would issue a warning, even if they are not being used in those source files.

With this new change, a non-kernel functor with this attribute invoked inside the kernel will not generate this warning and I am pretty certain nested lambdas will have the same issue. The more I think about it, I think it makes sense to only generate this warning only on functions called within the kernel. Why does it matter if the attribute is used on functions if there are never invoked in the kernel. It also would keep the behavior consistent between functions and lambdas and functors instead of diverging this way.

@dyniols
Copy link
Contributor Author

dyniols commented Oct 15, 2024

With this new change, a non-kernel functor with this attribute invoked inside the kernel will not generate this warning and I am pretty certain nested lambdas will have the same issue. The more I think about it, I think it makes sense to only generate this warning only on functions called within the kernel. Why does it matter if the attribute is used on functions if there are never invoked in the kernel. It also would keep the behavior consistent between functions and lambdas and functors instead of diverging this way.

Sure, I can change implementation. I assume you mean to issue warnings only in the device-code path?

@elizabethandrews
Copy link
Contributor

With this new change, a non-kernel functor with this attribute invoked inside the kernel will not generate this warning and I am pretty certain nested lambdas will have the same issue. The more I think about it, I think it makes sense to only generate this warning only on functions called within the kernel. Why does it matter if the attribute is used on functions if there are never invoked in the kernel. It also would keep the behavior consistent between functions and lambdas and functors instead of diverging this way.

Sure, I can change implementation. I assume you mean to issue warnings only in the device-code path?

Yes I think that makes more sense here unless @steffenlarsen disagrees.

@steffenlarsen
Copy link
Contributor

Yes I think that makes more sense here unless @steffenlarsen disagrees.

It does change the intention of the warning slightly, but I agree that with the exclusions we have to make to avoid user confusion it makes perfect sense to make it just a device-code path check.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Apr 17, 2025
@github-actions github-actions bot removed the Stale label May 17, 2025
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