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

adjacent_difference should not allow running with source being the same as destination #6890

Open
aprokop opened this issue Mar 20, 2024 · 12 comments · May be fixed by CExA-project/kokkos#10 or #6922
Open
Assignees
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos)

Comments

@aprokop
Copy link
Collaborator

aprokop commented Mar 20, 2024

Right now, it is possible to run

Kokkos::Experimental::adjacent_difference(space, view, view);

According to the standard, it should not be possible.

For the overloads with an ExecutionPolicy, the ranges [first, last)
and [result, result + (last - first)) shall not overlap.

Affects all Kokkos versions I tried.

@aprokop aprokop added the Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) label Mar 20, 2024
@masterleinad
Copy link
Contributor

Does this still generate the correct result? Is it a problem if the Kokkos implementation is more flexible than std::adjacent_difference?

@aprokop
Copy link
Collaborator Author

aprokop commented Mar 20, 2024

Does this still generate the correct result? Is it a problem if the Kokkos implementation is more flexible than std::adjacent_difference?

No, it does not generate correct result. It would be impossible to do that without duplicating the view, which it does not do.

For view [2, 4, 6, 8, 10], the currently produced result in Serial is [2, 2, 4, 4, 6].

@fnrizzi
Copy link
Contributor

fnrizzi commented Apr 4, 2024

on https://en.cppreference.com/w/cpp/algorithm/adjacent_difference
am i missing it or the precondition is not stated ?

@aprokop
Copy link
Collaborator Author

aprokop commented Apr 4, 2024

@fnrizzi yes, i didn't see it there when I submitted the issue. I only found it in the other reference.

Update: I guess they just did not copy the text from the standard. It's in it.

@dalg24
Copy link
Member

dalg24 commented Apr 4, 2024

on https://en.cppreference.com/w/cpp/algorithm/adjacent_difference am i missing it or the precondition is not stated ?

It does say

If op invalidates any iterator (including any of the end iterators) or modify any elements of the ranges involved, the behavior is undefined.

Update: I guess they just did not copy the text from the standard. It's in it.

Look at Andrey waving the Bible. The current draft has the same paragraph as in cppreference but it also has this remark https://eel.is/c++draft/adjacent.difference#8 that clearly spell out what the precondition involves.

@fnrizzi
Copy link
Contributor

fnrizzi commented Apr 5, 2024

Yes right, I was just surprised the other part of the condition was missing

@dalg24
Copy link
Member

dalg24 commented Apr 5, 2024

What do you mean when you say "the other part of the condition [is] missing"?

If op invalidates any iterator (including any of the end iterators) or modify any elements of the ranges involved, the behavior is undefined.

I expected that the fact the ranges cannot overlap was covered by the part of the clause I highlighted above.
You can check with @crtrott or @nliber, they are more fluent in standardese than I am.

@fnrizzi
Copy link
Contributor

fnrizzi commented Apr 5, 2024

I think what you highlight refers to what requirements "op" should have and so what it should do or not doto its arguments. How the result of op is used is a different thing IMO.

@fnrizzi
Copy link
Contributor

fnrizzi commented Apr 5, 2024

In any case, why would it be written so explicitly in working draft but so cryptically in the user documentation?

@yasahi-hpc yasahi-hpc linked a pull request Apr 6, 2024 that will close this issue
@jbigot
Copy link
Member

jbigot commented Apr 6, 2024

In any case, it is the responsibility of the user to ensure the preconditions. Here the standard makes no provision for the implementation of adjacent_difference to throw an exception, or signal an error in any way. The only thing that could be "fixed" is that the documentation could state the precondition more clearly.

In order to be a good citizen, Kokkos can try to check the precondition at runtime in certain simple cases and in debug mode ( Cf. @yasahi-hpc PR ), but this is just a nicety and goes beyond what the standard mandates.

@aprokop
Copy link
Collaborator Author

aprokop commented Apr 7, 2024

In any case, it is the responsibility of the user to ensure the preconditions.

Sure. I do think it falls under the same concept as checking array bounds: a helpful utility to detect errors at early stage.

@jbigot
Copy link
Member

jbigot commented Apr 7, 2024

Sure. I do think it falls under the same concept as checking array bounds: a helpful utility to detect errors at early stage.

Definitely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos)
Projects
None yet
6 participants