[SYCL] Solve name resolution problems due to Windows integer types

[mibintc]

There are 3 Windows CTS tests, math_builtin_relational_base,
math_builtin_float_double, math_builtin_integer that fail at compile
time in all 3 environments.

The error is like this,
math_builtin_float_double.cpp:8:12: error: no matching function for call to 'nan'
auto x = cl::sycl::nan(y);
^~~~~~~~~~~~~
...include\CL/sycl/builtins.hpp:398:1: note: candidate template
ignored: requirement 'detail::integral_constant<bool, false>::value' was not satisfied [with T = int]
nan(T nancode) __NOEXC {
^
1 error generated.

Here’s a smaller version of the invoker,

int foo(void)
{
//unsigned long int y = 1; FAIL like above
//long int y = 1; FAIL like above
//int y = 1; FAIL like above
unsigned y = 1; // This one is OK
auto x = cl::sycl::nan(y);
}

Erich suggested,

https://godbolt.org/z/XkWVHg

Even on MSVC, “unsigned int” and “unsigned long” are different types, despite sizeof being the same for both.

The header itself uses __int32 and __int64, which are just aliases on MSVC for “unsigned int” and “unsigned long long”.

THUS, the list in the headers (which I copied below) doesn’t contain ANY version of “unsigned long” on Windows. Ulonglong (the other being checked) is ALSO “unsigned long long”

On linux, the list is:
cl_uint == unsigned int
cl_ulong == unsigned long
ulonglong == unsigned long long.

Thus, the whole list is covered.

I suspect that one of the checks (is_ugenlong probably ) simply needs to have “unsigned long” added to it as an option in addition to cl_ulong. Alternatively, cl_ulong could be corrected.

Either way, a header issue.

Signed-off-by: Melanie Blower [email protected]

[premanandrao]

LGTM

[v-klochkov]

Melanie, I don't quite understand the problem and possible solutions here. I only heard there were at least two solutions, right? So, I would rather rely on other reviewers in this PR.

My question here is: why you used _MSC_VER to guard this code? I.e. not _WIN32 or even better SYCL_RT_OS_WINDOWS defined in os_util.hpp.

Also, what happens if you do not guard those lines at all, i.e. have/use same code for Windows & Linux? That probably would be the best if it works well.

[erichkeane]

Melanie said: If we didn't use conditional compilation, then we'd be getting similar name resolution compfail problems on Linux.

Are you sure? Long or cl_long should just be long or long on Linux.

[mibintc]

The reason that i use MSC_VER and not WIN32 is because it's the Microsoft compiler that decided to define the int types this way. It's conceivable that another windows compiler could define the int types differently.

If we didn't use conditional compilation, then we'd be getting similar name resolution compfail problems on Linux.

[erichkeane]

I've been asked to better explain the problem diagnosed here. First, the problem statement: 'cl::sycl::nan' called with an unsigned long fails a SFINAE check:

'error: no matching function for call to 'nan'
note: candidate template
      ignored: requirement 'detail::integral_constant<bool, false>::value' was not satisfied [with T =     unsigned long]

Next, the declaration of 'nan':

template <typename T, typename = typename std::enable_if<
                      detail::is_nan_type<T>::value, T>::type>
detail::unsign_integral_to_float_point_t<T>
nan(T nancode) {...}

Now is_nan_type:

template <typename T>
using is_nan_type =
std::integral_constant<bool, detail::is_ugenshort<T>::value ||
                                 detail::is_ugenint<T>::value ||
                                 detail::is_ugenlonginteger<T>::value>;

The idea here seems to be to enable is_nan_type for all unsigned integer types. is_ugenshort is:

typedef unsigned __int16 cl_ushort;
template <typename T>
using is_ushortn =
typename is_contained<T, type_list<cl_ushort2, cl_ushort3, cl_ushort4,
                                   cl_ushort8, cl_ushort16>>::type;
template <typename T>
using is_ugenshort =
std::integral_constant<bool, is_contained<T, type_list<cl_ushort>>::value ||
                                 is_ushortn<T>::value>;

This produces a 'true' value if the type is cl_ushort, or one of the cl_ushortN types.

Now is_ugenint, which is equally unexciting:

typedef unsigned __int32 cl_uint;
template <typename T>
using is_uintn = typename is_contained<
  T, type_list<cl_uint2, cl_uint3, cl_uint4, cl_uint8, cl_uint16>>::type;
template <typename T>
using is_ugenint =
  std::integral_constant<bool, is_contained<T, type_list<cl_uint>>::value ||
                                 is_uintn<T>::value>;

HOWEVER, is_ugenlonginteger is where things get interesting.

typedef unsigned __int64 cl_ulong;
typedef unsigned long long ulonglong;
template <typename T>
using is_ulongn = typename is_contained<
  T, type_list<cl_ulong2, cl_ulong3, cl_ulong4, cl_ulong8, cl_ulong16>>::type;
template <typename T>
using is_ugenlong =
  std::integral_constant<bool, is_contained<T, type_list<cl_ulong>>::value ||
                                 is_ulongn<T>::value>;
template <typename T>
using is_ulonglongn =
  typename is_contained<T, type_list<ulonglong2, ulonglong3, ulonglong4,
                                   ulonglong8, ulonglong16>>::type;
template <typename T>
using is_ugenlonglong =
  std::integral_constant<bool, is_contained<T, type_list<ulonglong>>::value ||
                                 is_ulonglongn<T>::value>;
template <typename T>
using is_ugenlonginteger =
  std::integral_constant<bool,
                       is_ugenlong<T>::value || is_ugenlonglong<T>::value>;

Note that the "is_ugenlonginteger" checks "is_ugenlong" (which is equally as uninteresting as "is_ugenshort and is_ugenint"). However, it ALSO includes include isugenlonglong. The idea here is that all unsigned integer types are covered here. This works on linux because of these lines:

typedef unsigned __int16 cl_ushort; // short
typedef unsigned __int32 cl_uint;    // int
typedef unsigned __int64 cl_ulong; // long
typedef unsigned long long ulonglong; // long long

Those types end up being the entire collection of int types in C++, __int16==short, __int32==int, __int64==long, long long ==long long. IMO, I suspect that "ulonglong" being defined to be "long long" is simply to "cheat" the fact that 'long long' is typically 64 bits on linux, so there is no size-based spelling of it.

HOWEVER, on Windows, the completeness of the integral types breaks here:

typedef unsigned __int16 cl_ushort; // short
typedef unsigned __int32 cl_uint;    // int
typedef unsigned __int64 cl_ulong; // NOW long long
typedef unsigned long long ulonglong; // long long

Since on Windows "long" is 32 bit (for winapi compatibility reasons), __int64 is long long. So the check of : "is cl_ushort || is cl_uint || is cl_ulong || is cl_ulonglong" is actually no longer "ushort || uint || ulong || ulonglong", it is now: " ushort ||uint || ulonglong || ulonglong".

Note that unsigned long long ends up being checked twice, but unsigned long is never checked. It used to come from cl_ulong, now it doesn't.

Since the intent is to check for all ints of roughly that size, we need to add those checks SOMEWHERE. I'm not terribly sure where the RIGHT place is here. I'm hoping someone more familiar with this header has a good idea of what the actual intent of these types is.

Its a little cruel to reuse the C integer type names, yet have them be defined to be fixed-size, since you can very easily (on windows with long) end up with a type that is a different size than the C version.

My suggestion to Melanie was simply to make sure that the checks for 'long' included the actual type of 'long', which is what she did for is_genlong and is_ugenlong. Upon further reflection, I suspect the #ifdef _MSC_VER might be unnecessary, since "long || cl_long" are the same type on Linux, so it is simply redundant.

[v-klochkov]

These 'long' types definitions and uses are so fragile, i.e. it is really easy to break something.
Thus it would be good to have a LIT test (or extend some existing LIT test) to see possible regressions early.

[mibintc]

@v-klochkov There aren't any LIT tests that are checking the sycl include files.

[v-klochkov]

@v-klochkov There aren't any LIT tests that are checking the sycl include files.
I don't understand this... What LIT tests check then?

I mean LIT tests like Serguei added here: 336cb82
That was a fix in sycl include file + lit test.

It is not for sure, but that fix could potentially be the cause of regression in 6 CTS tests for long types (even though fixed 10 CTS tests for long_long types). Adding LIT test will guarantee that the same problem Serguei already fixed will not return with some other harmless fix.

[mibintc]

I made some git mistakes in the latest revision, so i uploaded a clean PR here, #523

This newer version doesn't use MSC_VER

[mibintc]

Ok thanks, I never added tests in that part of the tree before. Thanks for the info. From: v-klochkov [mailto:[email protected]] Sent: Friday, August 16, 2019 5:58 PM To: intel/llvm <[email protected]> Cc: Blower, Melanie <[email protected]>; Author <[email protected]> Subject: Re: [intel/llvm] [SYCL] Solve name resolution problems due to Windows integer types (#516) @v-klochkov<https://github.com/v-klochkov> There aren't any LIT tests that are checking the sycl include files. I don't understand this... What LIT tests check then? I mean LIT tests like Serguei added here: 336cb82<336cb82> That was a fix in sycl include file + lit test. It is not for sure, but that fix could potentially be the cause of regression in 6 CTS tests for long types (even though fixed 10 CTS tests for long_long types). Adding LIT test will guarantee that the same problem Serguei already fixed will not return with some other harmless fix. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#516?email_source=notifications&email_token=ACLOUBUUNCGDTIO27MWVDV3QE4PIHA5CNFSM4IL7ZOE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4PZQKA#issuecomment-522164264>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACLOUBTEYJCSQTJGHEUAOXLQE4PIHANCNFSM4IL7ZOEQ>.

[bader]

According to the comment above, cl_ulong is not ugenlong.

Also I think we should update the logic of ALL traits that are not relying on fixed size types.
We probably don't see these issues now, but we might hit them on some other platforms.
This can be validated by the cross-compilation for the triples, which defines sizes of char != 1, short != 2 and int != 4.

[v-klochkov]

Good catch. This though does not relate to Melanie's fix. At least it is some older issue, right?
How about fixing that cl_ulong issue and 'update the logic of ALL traits' separately?

[romanovvlad]

@alexeyvoronov I believe your patch contains fixes for the same problem this patch does. Could you, please, confirm?

[v-klochkov]

I believe this PR is outdated and should be simply closed after Serguei's fix here: #545

[romanovvlad]

Ok, closing PR.