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

Revert PR#222 with incorrect fix for #218 #249

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mzuzek
Copy link

@mzuzek mzuzek commented Mar 8, 2023

This PR reverts #222 (commit 098f6ef) with partial fix for #218:
the implementation was found out to be incorrect - see #248 and #218 for more details.

…ads for transposed and scaled inputs kokkos#218"

This reverts commit 098f6ef.
@mzuzek mzuzek added the bug Something isn't working label Mar 8, 2023
@mzuzek mzuzek requested review from mhoemmen and fnrizzi March 8, 2023 11:47
@mzuzek mzuzek self-assigned this Mar 8, 2023
Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

I'll leave this up to @crtrott , who is more familiar with the customization design.

@fnrizzi
Copy link
Contributor

fnrizzi commented Mar 8, 2023

I'll leave this up to @crtrott , who is more familiar with the customization design. Here's what I wrote on #218.

I do not like that the reference implementation is trying to ADL-find some customization points with the same names.
The proposal is still in flight. LEWG asked us to change function names in R12. When we make those changes here, we will break whatever code depends on those names.
The Right Way is to separate stdBLAS's public interface from its implementation. Any customization points should have DIFFERENT NAMES. Users should not have to worry about compiler errors because the call to matrix_vector_product is ambiguous between std::linalg::matrix_vector_product and my_funny_namespace::matrix_vector_product.

where is your comment in the issue? it disappeared?

@mhoemmen
Copy link
Contributor

mhoemmen commented Mar 8, 2023

@fnrizzi wrote:

where is your comment in the issue? it disappeared?

I deleted the comment, because it wasn't accurate. Never mind : - ) Christian and I are discussing the solution now.

@crtrott
Copy link
Member

crtrott commented Mar 8, 2023

Basically the infinite recursion should only happen if the specialization (i.e. the Kokkos variant) isn't more special than the generic one which is completely unconstrained on the ExecPolicy see https://godbolt.org/z/7x5hhne8o

Basically the specialized implementation needs to have the same constraints as the entry function (i.e. you need to take the mdspans as mdspans and be templated on all the other things). In that way this won't recurse.

@fnrizzi
Copy link
Contributor

fnrizzi commented Mar 8, 2023

Basically the infinite recursion should only happen if the specialization (i.e. the Kokkos variant) isn't more special than the generic one which is completely unconstrained on the ExecPolicy see https://godbolt.org/z/7x5hhne8o

Basically the specialized implementation needs to have the same constraints as the entry function (i.e. you need to take the mdspans as mdspans and be templated on all the other things). In that way this won't recurse.

I was going to say I agree with differentiating names, and in fact I brought it up a while back, but was kind of "not a pressing issue" at that time I guess. why is ADL a better solution?

@mhoemmen
Copy link
Contributor

mhoemmen commented Mar 8, 2023

@fnrizzi Just to clarify: Christian and I did some investigation, and we think the actual issue could be that your custom matrix_vector_product doesn't sufficiently constrain its parameters.

@fnrizzi
Copy link
Contributor

fnrizzi commented Mar 8, 2023

@fnrizzi Just to clarify: Christian and I did some investigation, and we think the actual issue could be that your custom matrix_vector_product doesn't sufficiently constrain its parameters.

yes, ok, i see it could be the case also from what Christian said. But this being said, I don't quite understand why not using a different name? Effectively, this would also express better(IMO) the intent since one is writing an implementation of the stdlinalg and those impl functions should in theory not even be used directly. That would take away all issues and it seems to me that relying on ADL and the fact that customizations must be constrained enough/well is a kind of a delicate thing, very easy to mess up. I do get that it has to be done once, but still, any small change can inadvertently break this.

@mhoemmen
Copy link
Contributor

mhoemmen commented Mar 8, 2023

@fnrizzi wrote:

But this being said, I don't quite understand why not using a different name? Effectively, this would also express better(IMO) the intent since one is writing an implementation of the stdlinalg and those impl functions should in theory not even be used directly. That would take away all issues and it seems to me that relying on ADL and the fact that customizations must be constrained enough/well is a kind of a delicate thing, ....

Users want to customize algorithms like matrix_vector_product, and users also want the stdblas implementation to find customizations if they exist. However, if users get the customization wrong (e.g., incorrect constraints), then bad things can happen.

Choosing a different name for the customization point than the function would prevent infinite recursion issues like #218. However, it wouldn't prevent other issues. For example, if users implementing a customization don't constrain its parameters correctly or provide the wrong number of parameters, then the compiler might silently ignore the customization.

P2279 has a survey of different customization approaches. It explains the different ways that each of them can go wrong, and argues for a language solution. P2547 is an example of a proposed language solution.

I wouldn't object to making the customization points have different names. However, this would be a breaking change, yet it would only solve one of the many issues with relying on ADL as a customization mechanism. If we plan to make a breaking change, then we might want to consider adopting a different approach for customization.

@fnrizzi
Copy link
Contributor

fnrizzi commented Mar 8, 2023

@mhoemmen yes thanks for those links, very useful :) I will read them.

Re "However, it wouldn't prevent other issues. For example, if users implementing a customization don't constrain its parameters correctly or provide the wrong number of parameters, then the compiler might silently ignore the customization."

Yea totally agree! the naming would at least prevent one issue which was IMO a tricky one. I was actually reasoning from the assumption that the customization mechanism was "decided" except for the naming... if the way we customize can be changed or discussed then this changes things. At least changes where I start to think from :)
I will think a bit more about this

@mhoemmen
Copy link
Contributor

mhoemmen commented Mar 8, 2023

@fnrizzi wrote:

I was actually reasoning from the assumption that the customization mechanism was "decided" except for the naming...

Just to clarify: the customization mechanism is not part of the proposal P1673. It's specific to this implementation of the proposal. Thus, it can be changed independently of the proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants