32-bit generic target broken (x86) due to not finding a 64-bit integer type for (u)long

[Pekka Jääskeläinen]

There should be automated detection for int64_t etc. in the "generic target". Currently x86 compilation is broken as double is found but 64bit int not. I think the proper way to fix it is to use a config check for finding if stdint.h and a 64bit int is found and not just failing to support 64bit ints if 'long' is not 64 bits.

[Carlos Sánchez de La Lama (csanchezdll)]

I have done a build system rework to allow compilation for arbitrary host/target triplets, inclusing cross-clang detection, so this should be fixed. I will commit as soon as I am able to merge.

[Pekka Jääskeläinen (pekka-jaaskelainen)]

OK, well I only wasted a couple of hours with this today ;)

Here is the patch I managed to make. The problem I saw is that long is defined as fixed width 64 bit type in OpenCL C and in C they can vary. Therefore I use 'slong' in _kernel.h until the end and let the target-specific types.h define __int64_t to be the fixed width type however they see best. In the "generic" types.h I use stdint.h to try to get the int64_t from there.

[Pekka Jääskeläinen (pekka-jaaskelainen)]

Your r127 fixes the compilation of pocl but doesn't fix the issue I started to fix in my patch; the false assumption that the C 'long' type should be 64bit, otherwise disable support 64 bit integers in OpenCL C for the 'native'. It doesn't try to find a proper 64bit integer type and use it in _kernel.h instead, which I think it should.

The commit breaks 'example1' (just segfaults in the kernel function, I didn't investigate further yet). The scalarwave example fails due to missing double in x86. My P4 supports doubles (I think in HW) so double gets disabled due to disabling the 64bit ints (which are supported via emulation AFAIK, but should still allow using them I think).

So, maybe I should apply the parts of my patch that add an abstraction to the integer type 'long' in _kernel.h next week if you are not working on something similar to that too.

Can you please add file comments to the several types.h.* etc. as the purpose of those is quite confusing (host/target, etc.) and is getting more so (I think I know their purposes but the next code reader might not), thanks!

[Pekka Jääskeläinen (pekka-jaaskelainen)]

Some issues with installation aswell?

00:42 < kraiskil> there is something fishy going on with csanchez's last merge on a 32bit x86... builds but doesn't install... works, but doesn't pass tests

[Carlos Sánchez de La Lama (csanchezdll)]

The example1 breakage is fixed... the alignment tests were running using gcc instead of clang so it was wrongly detected (old bug).

[Pekka Jääskeläinen (pekka-jaaskelainen)]

scalarwave still broken on x86-32 due to the issue I mentioned above. Shall I apply the part of my patch that abstracts the 64bit integer type, or are you working on another solution for this?

[Carlos Sánchez de La Lama (csanchezdll)]

Yes, last commit did was not supposed to fix scalar wave in x86. Probably some changes are needed in your patch with the new build system, I was trying to get some time to look at it but you can apply if in a hurry. Try to make sure it compiles for other --targets (I am using --target=mips myself).

[Pekka Jääskeläinen (pekka-jaaskelainen)]

I fixed the installation.

[Pekka Jääskeläinen (pekka-jaaskelainen)]

This needs fixing at Clang side, not high priority as pocl still works for the x86_32 but only has the double and int64 disabled (therefore the scalarwave fails).

[Pekka Jääskeläinen (pekka-jaaskelainen)]

For the Clang part, see: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-January/019505.html

[Erik Schnetter (schnetter)]

I don't think this requires changes to Clang (although it should be implemented there). I'm thinking of
{{{
typedef int64_t _cl_long;
typedef uint64_t _cl_ulong;
#define long _cl_long
#define ulong _cl_ulong
}}}
in types.target.in and types.host.in.

[Pekka Jääskeläinen (pekka-jaaskelainen)]

Yes this could work (at least improve the current situation), but I recall wondering what to do with "unsigned long" etc. with the two part type names. That would result to "unsigned int64_t" which does not work to my knowledge.

[Erik Schnetter (schnetter)]

The full code would be:

#if @TARGET_SIZEOF_LONG@ == 8
# define cles_khr_int64
typedef long _cl_long;
typedef unsigned long _cl_ulong;
#elif @TARGET_SIZEOF_LONG_LONG@ == 8
# define cles_khr_int64
typedef long long _cl_long;
typedef unsigned long long _cl_ulong;
#elif @TARGET_SIZEOF_INT64_T@ == 8
# define cles_khr_int64
typedef int64_t _cl_long;
typedef uint64_t _cl_ulong;
#else
/* Type "long" is not supported */
# define __EMBEDDED_PROFILE__ 1
# undef cles_khr_int64
#endif
#define long _cl_long
#define ulong _cl_ulong

which also requires changes to configure.ac:

AC_CHECK_SIZEOF([long long])
AC_SUBST([TARGET_SIZEOF_LONG_LONG], [$ac_cv_sizeof_long_long])
AC_CHECK_SIZEOF([int64_t])
AC_SUBST([TARGET_SIZEOF_INT64_T], [$ac_cv_sizeof_int64_t])

[Erik Schnetter (schnetter)]

Actually, this does not quite work, because we use the preprocessor to generate type names and routine names. This leads to a confusion between e.g. the type name "long2" and "_cl_long2". This could probably be corrected by additional #defines.

Also, error messages involving "long" are then printed as "_cl_long", which may be confusing.

Is this worth the effort?

[Pekka Jääskeläinen (pekka-jaaskelainen)]

OK. Seems like something similar I started to do in the above patch.

So...

a) for 32-bit, [unsigned] long is just unsupported (what does _cl_[u]long expand to in that case?)
b) for 64-bit "unsigned long" --> "unsigned _cl_long" --> "unsigned long" | "unsigned long long".

Should work. Please test that 'make check' passes for you and commit. I can test the 32-bit functionality later with my Pentium 4 back at home. The build could just disable the double/long requiring tests in that case for make check to get success on 32bit.

[Pekka Jääskeläinen (pekka-jaaskelainen)]

True. Unsure about the worth, maybe it's easiest and cleanest to try to propose the change I suggested in the unanswered Clang mail.

[Pekka Jääskeläinen (pekka-jaaskelainen)]

BTW seems that in my patch I have started to go towards something you describe (rename the type used in the macros). I recall battling with it for a while and then stopped in frustration.

[Pekka Jääskeläinen (pekka-jaaskelainen)]

Some progress in Clang upstream: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20121119/068546.html

[Pekka Jääskeläinen (pekka-jaaskelainen)]

This is now fixed thanks to Erik's patches to Clang.