[PATCH] D28080: [Docs][OpenCL] Added OpenCL feature description to user manual.

Wed Jan 11 05:22:12 PST 2017

Anastasia marked 16 inline comments as done.
Anastasia added inline comments.

================
Comment at: docs/UsersManual.rst:2013
+to perform machine code generation.
+
+Clang currently supports OpenCL standards up to v2.0.
----------------
pekka.jaaskelainen wrote:
> I'd like to add a sentence or two for instructing how to use it for non-SPMD targets and of course advertise pocl here too ;) 
> 
> Something along the lines of:
> 
> "For "non-SPMD" targets which cannot spawn multiple work-items on the fly using hardware, which covers practically all non-GPU devices such as CPUs and DSPs, additional processing is needed for the kernels to support multiple WI execution. For this, a 3rd party project such as `pocl <http://portablecl.org/>`_ can be used."
Cool! I have added this later in the description of x86 target use for OpenCL. :)

================
Comment at: docs/UsersManual.rst:2053
+
+Disables standard target extensions. All OpenCL targets provide a list
+of extensions that they support. Clang allows to amend this using the ``-cl-ext``
----------------
yaxunl wrote:
> Maybe rephrase as "Disables support of OpenCL extensions" instead of "Disable standard target extensions" since "standard target extension" is confusing and also to avoid confusion with the effect of disabling OpenCL extension as in '#pragam OPENCL EXTENSION X: disable'. Disabling support of an extension means that they cannot be enabled by pragmas in the OpenCL kernels since they are deemed as non-supported.
Good point!

================
Comment at: docs/UsersManual.rst:2114
+     $ clang -target nvptx64-unknown-unknown test.cl
+
+Generic Targets
----------------
pekka.jaaskelainen wrote:
> This is an alternative location for my pocl advertisement above with a subtitle:
> 
> - For CPU/DSP architectures such as x86, ARM or TCE:
> ...
> 
Added later in the x86 description. 

================
Comment at: docs/UsersManual.rst:2120
+  that can be used across GPU toolchains. The implementation follows `the SPIR
+  specification <https://www.khronos.org/spir>`_. There are two flavors available
+  for 32 and 64 bits.
----------------
bader wrote:
> pekka.jaaskelainen wrote:
> > Which version of SPIR is generated?
> For -cl-std=CL1.x (where x is 0, 1 or 2), SPIR version is 1.2.
> For -cl-std=CL2.0, SPIR version is 2.0.
Is it worth mentioning this in this doc?

================
Comment at: docs/UsersManual.rst:2130
+
+- x86 is used by some implementations that are x86 compatible
+  (e.g. `POCL <http://portablecl.org/>`_) and currently remains for backwards
----------------
pekka.jaaskelainen wrote:
> This is a bit confusing paragraph, probably due to my confusing explanations of the problems with pocl. Currently pocl tries not to use FASM for preserving logical AS IDs to LLVM IR due to the bunch of problems it constantly produces with seemingly little benefits for common CPUs. My patch related to this considered only the argument info switch. Now pocl only derives the logical iDS from kernel arguments (and all other IDs within the body of the IR function are lost for flat machines).  In my patch, the argument info's address space IDs were made constant and identical to SPIR's as previously they were the target's (which meant losing the AS IDs altogether for flat AS machines).
> 
> You seem to document the arg-info md switch later, so this paragraph might be removed or converted to my pocl blurb which mentions the need for further processing for CPUs.
Yes, it's perhaps a bit confusing indeed. I find the usage of the x86 backend for OpenCL is generally a bit confusing. Also there are a lot of program paths even in Clang that don't play well for OpenCL purposes or are not handled properly and are therefore a source of bugs.

I was just wondering whether it would be possible to switch to using SPIR as a generic target at some point in the future and "deprecate" the x86 target for OpenCL . Do you think this might work for POCL? At the same time we have upcoming SPIR-V support as a new feature that might reshape things.

Regarding the address space I just know that it's common to use fake address space map with the x86 backend for some out of tree implementations to add missing memory segments to this target. That's why I added this text here.

================
Comment at: docs/UsersManual.rst:2142-2143
+
+By default Clang will not include standard headers and therefore OpenCL builtin
+functions are unknown. The default CL header is, however, provided in the Clang
+installation and can be enabled by passing the ``-finclude-default-header`` flag
----------------
bader wrote:
> Not sure it's worth to mention, but even built-in vector types are defined in the opencl-c.h.
Good point!

================
Comment at: www/index.html:19
+  <p>The goal of the Clang project is to create a new C based language
+  front-end: C, C++, Objective C/C++, OpenCL and others for the
+  <a href="http://www.llvm.org/">LLVM</a> compiler.  You can
----------------
pekka.jaaskelainen wrote:
> OpenCL C. Maybe OpenCL C++ also?
We don't have OpenCL C++ officially yet. :(

https://reviews.llvm.org/D28080