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

TeamThreadRange requires begin, end to be the same type #305

Closed
alanw0 opened this issue May 26, 2016 · 8 comments
Closed

TeamThreadRange requires begin, end to be the same type #305

alanw0 opened this issue May 26, 2016 · 8 comments
Assignees
Labels
Enhancement Improve existing capability; will potentially require voting
Milestone

Comments

@alanw0
Copy link

alanw0 commented May 26, 2016

int begin = 0;
unsigned end = 1000;
TeamThreadRange(team, begin, end)

error:

error: no instance of overloaded function "Kokkos::TeamThreadRange" matches the argument list
            argument types are: (const TeamHandleType, int, const unsigned int)
@mhoemmen
Copy link
Contributor

mhoemmen commented May 26, 2016

Hi @alanw0 -- I don't agree that this is actually an issue with Kokkos. How is Kokkos supposed to know what index type you meant to use for the loop? Is it int or unsigned? Which one should it pick? If Kokkos were to prefer one over the other, I'm sure some user would get annoyed. It's not Kokkos' job to impose a preference for signed or unsigned integer types.

Suppose that you wrote the following for loop:

int begin = 0;
unsigned end = 1000;
for (int k = begin; k < end; ++k) {
  // ... do something ...
}

The compiler would rightfully warn about your signed / unsigned comparison. Ditto if you used unsigned for k instead of int.

@crtrott
Copy link
Member

crtrott commented May 26, 2016

Mark I actually told Alan to put this in, i.e. I was with him when we ran into this. I think we should fix this that it would work, but it might issue the warning you talk about. But just refusing to compile might be a bit harsh.

@hcedwar
Copy link
Contributor

hcedwar commented May 26, 2016

Agree with Mark on this one. For clarity we could emit a static_assert when the types don't match.

@alanw0
Copy link
Author

alanw0 commented May 26, 2016

Christian understands the issue better than I do.
But to reply to Mark's code snippet, as a kokkos user I don't write the "for(int k..." loop, I just write the "... do something..." body. But I do specify the argument "int k" to the lambda or operator()() that contains the "do something". I guess the equivalent of the for-loop containing the "k < end" lies in kokkos somewhere. I think I agree with Christian's point that a compile-warning would be fine, and would still let me know what my mistake was.

@ndellingwood ndellingwood added the Enhancement Improve existing capability; will potentially require voting label Jun 8, 2016
@ndellingwood
Copy link
Contributor

Catch error with a static_assert

@ndellingwood ndellingwood added this to the Summer 2016 milestone Jun 8, 2016
@ndellingwood ndellingwood self-assigned this Jun 8, 2016
@hcedwar hcedwar assigned gmackey and unassigned ndellingwood Jul 20, 2016
@hcedwar
Copy link
Contributor

hcedwar commented Jul 20, 2016

Evaluate the two types and choose the appropriate type according to "larger" type rule.

@hcedwar hcedwar modified the milestones: Fall 2016, Summer 2016 Aug 31, 2016
@hcedwar
Copy link
Contributor

hcedwar commented Sep 28, 2016

Investigate use of C++11 std::common_type<intX,intY>::type

@gmackey
Copy link
Contributor

gmackey commented Oct 4, 2016

Using std::common_type is the right approach because it does what a user would expect (aka what the C++ language would do). It uses the common type that C++ would implicitly convert multiple types to. If the two types are different sizes, it chooses the type of the larger size. If the two types are of the same size but one is signed and the other is unsigned, it chooses the unsigned type.

We had considered making the type be signed if it was 64 bit for performance reasons, but I think it's better to use a built-in C++ feature vs. writing our own. It's also better to do it the way the user would expect.

hcedwar added a commit that referenced this issue Oct 5, 2016
TeamThreadRange now allows begin and end to be different types (#305)
@crtrott crtrott closed this as completed Oct 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

6 participants