Hey Erik, Awesome patch, much more extensive than anything I would do :).
My only concern at the moment is about the address space map - how do we reconcile us forcing the address space map for OpenCL with a target that already has an address space map (like the PTX target). The ASTContext already has a way to have a fake address space map using the LangOptions field FakeAddressSpaceMap - perhaps we could just in setForcedLangOptions turn that language option on for OpenCL (assuming of course that setForcedLangOptions is always called before the first query to getAddressSpaceMap from ASTContext).
Cheers, -Neil. On 31/08/2013 20:05, Erik Schnetter wrote:
OpenCL C requires that the type "long" always has 64 bits, independent of the architecture. Without proper support for "long", "double" can also not be supported in OpenCL C, so correcting this is important. A patch to implement this was suggested some time ago, but was not committed. I have extended this patch, and tested it on both x86_64 and i386. I attach the patch below. This patch consists of four hunks. The first uses PointerWidth instead of LongWidth to determine whether an architecture uses 32-bit or 64-bit registers. Using LongWidth does not work with OpenCL C since long there has always 64 bits, whereas I expect all architectures to use the same number of bits for a C long and a C pointer. The second hunk overrides some target-specific choices for type widths in TargetInfo::setForcedLangOptions when the language is OpenCL C. These widths are required by OpenCL C. As extension to OpenCL C, I have set long long to be a 128-bit type. This is described by the OpenCL standard in a "reserved for future use" section. The choice would be either to disable long long (how?), or to make it a 128-bit type. The latter seemed easier. The last two hunks set two command-line options that are also required for OpenCL C: "char" is always signed, and "errno" does not exist. Other parts of Clang artificially set command line options depending on the target architecture (and independent of the language), so this patch ignores the respective command line options when the language is OpenCL C. In the future, "-fno-builtins" may also be added to this -- I am not quite sure what this option does or why it is necessary, but we currently always use it when compiling OpenCL C code. The original patch caused some discussion, and I want to answer to some of the points raised in this discussion: (1) One suggestion is that changing sizeof(long) is actually defining a new target triple, and should be implemented as such. I want to disagree with this. First, OpenCL kernels do not have access to libc, operating system calls, or anything where the "usual" ABI for a system would matter -- OpenCL kernels are self-contained. Thus, introducing a new target triple seems overkill. Second, all that OpenCL C does is use the name "long" for a type that is called "long long" in C. The name used in the source code of a language is independent of the ABI. This is also evident from the fact that this patch only changes Clang, and not LLVM -- the emitted byte code will use the type "i64", and the back-end never knows whether the type was called "long" or "long long" in the source code. (2) At another point, the question was raised whether changing the meaning of "long" could break the target-lowering code that converts from byte code to machine language. Again, this seems impossible since byte code contains no reference to the type name, and uses integer and bit-widths only. As to whether setForcedLangOptions is a good place for this patch -- I don't know, as I am not an expert in Clang. Other places seem possible, e.g. the TargetInfo constructors. However, putting the code there would mean that (a) Opts.OpenCL needs to be available there (it currently isn't), and (b) all TargetInfo constructors need to remain in sync with the OpenCL C specification. One suggestion was to split TargetInfo into two, creating a new class LanguageInfo, where presumably C and C++ would look at TargetInfo to choose their size for "long", whereas OpenCL would not. This may be elegant, but given that the current patch has only about 20 lines I wonder whether this is overkill as well. To think about this, maybe one should consider other potential future languages for Clang (CUDA? UPC?). I would appreciate feedback. -erik _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
