[cfe-dev] OpenCL support

[Peter Collingbourne]

Hi Anton,

Thanks for the update and the new patches.

On Thu, Dec 16, 2010 at 06:16:25PM -0000, Anton Lokhmotov wrote:
> Hi Peter and Mike,
> 
> Many thanks for your comments!
> 
> > > @@ -1192,6 +1192,8 @@ private:
> > >    bool IsTrivial : 1; // sunk from CXXMethodDecl
> > >    bool HasImplicitReturnZero : 1;
> > >
> > > +  bool IsOpenCLKernel : 1;
> > 
> > A better way of storing this information would be to use an attribute
> > (Attr subclass).  See the include/clang/Basic/Attr.td file.
> > ...
> > We should only add a new field if we expect it to be used often.
> 
> We have considered this option but attributes have too complicated a
> semantics which would make handling this qualifier unwieldy.  Bit-fields
> probably get stored in a 32-bit word, which means there are 20 more unused
> bits in every FunctionDecl object anyway.  We do use Clang attributes for
> the optional function qualifiers:
>    * __attribute__((vec_type_hint(<typen>)))
>    * __attribute__((work_group_size_hint(X, Y, Z)))
>    * __attribute__((reqd_work_group_size(X, Y, Z)))

We may in future want to merge FunctionDecl fields into Decl, which
will give us even less space to work with.  We also plan to track
qualifier locations in the AST, which would add at least 32 bits to
the size of FunctionDecl.  If this were an attribute the location
could be stored as an Attr field.

There are several examples of existing attributes in the Clang source
code which you should be able to follow.  Perhaps you can explain in
more detail why you cannot use attributes here?

> Mike Gist wrote:
> > You could also consider placing all kernel functions in a 'kernel'
> > section, or adding a function attribute for kernels.
> Unlike in Clang, function attribute bit-fields in LLVM are pretty crowded
> (only couple of bits are unused?).  Besides, we do not want to represent
> differently the kernel qualifier and optional kernel qualifiers, which
> require storing <typen> and X, Y, Z values. (I don't even want to think how
> one would mangle the optional qualifiers into a kernel name and then
> demangle.)
> 
> That's why we propose to use metadata for this purpose.  Does anyone have a
> better idea?

I'll reply to this in a separate mail to llvmdev+cfe-dev.

> Peter Collingbourne wrote:
> > I don't think it is a good idea to use LangOptions for this.
> > LangOptions is an input parameter so we shouldn't modify it during
> > parsing/semantic analysis.  Modifying the LangOptions could also affect
> > clients which reuse a LangOptions object expecting it to be unchanged.
> We have addressed this by using an OpenCLOptions struct in Preprocessor.

I still think that Sema is the best place to store this information.
That's because it is the only part of the system which needs to care
about which OpenCL extensions are enabled at any given time.  The way
I imagined it would work would be to add a new pair of actions to Sema
(ActOnPragmaFPContract, ActOnPragmaCLExtension perhaps?)  which you
could call from your pragma handler.  Those actions would then set
the appropriate Sema/OpenCLOptions fields.

> Please find attached the rework of the first patch plus the second patch on
> OpenCL keywords.

More detailed comments on the first patch below...

> +  unsigned cl_fp_contract            : 1; // OpenCL FP_CONTRACT state.

I'm not sure this should be an OpenCL-specific option.  In future
we would like to support #pragma STDC FP_CONTRACT, and ideally we'd
like to be able to reuse OpenCL's FP_CONTRACT support.  I'd suggest
just making this a Sema field.

> +    // OpenCL1.1 enables these extensions by default
> +    cl_khr_global_int32_base_atomics =
> +      cl_khr_global_int32_extended_atomics =
> +      cl_khr_local_int32_base_atomics = 
> +      cl_khr_local_int32_extended_atomics =
> +      cl_khr_byte_addressable_store = 1;

I presume you misread Table 4.3 from the spec, which states
that those extensions must be supported, which is not the same
thing as being enabled.  In fact, section 9.1 states:

"The initial state of the compiler is as if the directive

             #pragma OPENCL EXTENSION all : disable

was issued, telling the compiler that all error and warning reporting
must be done according to this specification, ignoring any extensions."

Are you planning to have a mechanism for specifying which extensions
are supported by a particular implementation?  My initial thoughts
are that we can add a member function to TargetInfo which returns an
OpenCLOptions struct containing this information.

> +  SourceLocation CLLoc = CLTok.getLocation();
> +  
> +  Token Tok;
> +  PP.Lex(Tok);
> +  if (Tok.isNot(tok::identifier)) {
> +    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_identifier)
> +      << "OPENCL";
> +    return;
> +  }
> +  
> +  // Choose between EXTENSION and FP_CONTRACT
> +  
> +  IdentifierInfo *cmd = Tok.getIdentifierInfo();
> +  
> +  if(cmd->isStr("FP_CONTRACT")) {

You should split this PragmaHandler into separate handlers for
#pragma OPENCL FP_CONTRACT and #pragma OPENCL EXTENSION.  Then
there's no need to do the string matching yourself, and it will
be easier to reuse the FP_CONTRACT support.

Comments on the second patch to come...

Thanks,
-- 
Peter