[cfe-commits] cfe-commits Digest, Vol 50, Issue 74

[Peter Collingbourne]

On Sun, Aug 14, 2011 at 12:16:59PM +0100, Anton Lokhmotov wrote:
> > Updated patch attached.  I'll commit in a week unless there are
> > any objections.
> 
> Hi Peter,
> 
> +    SC_OpenCLWorkGroupLocal,
> Do you intend to introduce a storage class specifier for variables declared
> in the constant address space?  If yes, I think it would be better to use
> SC_OpenCLLocal and SC_OpenCLConstant as the names.  If not, why not?

I assume you mean "storage class" here, rather than "storage class
specifier"?  I don't think we need another storage class for __constant;
I believe that static should suffice for any implementation.

> 
> +  case SC_OpenCLWorkGroupLocal: return "work-group-local"; break;
> +  case SC_PrivateExtern:        return "__private_extern__"; break; 
> > Because using this "storage class specifier" in source code would be a
> > mistake, I deliberately chose a spelling that would not lex to a single
> > token, but that would still be useful for AST dumper tools.
> Perhaps you could use "__work_group_local__" to be consistent with
> SC_PrivateExtern?

But this would be a single token, and using it could mislead people
into thinking they can use __work_group_local__ in their code (as they
can with __private_extern__).  Because SC_OpenCLWorkGroupLocal is the
only storage class without a specifier, I don't think we should make
it consistent with any other class here.  In fact, I'm thinking we
should emphasise the distinction by calling it "<<work-group-local>>"
or some such.

> 
> +  CGOpenCLRuntime.cpp
> Could you please provide an overview of this functionality?  How do you plan
> to develop it.  How one would use it?

One would develop support for a given OpenCL implementation
by implementing a subclass of CGOpenCLRuntime, and modifying
CodeGenModule::createOpenCLRuntime to return an instance of this
subclass when appropriate.  The base class provides an example of a
runtime that uses internal globals for work-group-local variables,
and this can be changed by overriding the EmitWorkGroupLocalVarDecl
function.

The main requirement for EmitWorkGroupLocalVarDecl is that it add
the appropriate entry to LocalDeclMap.  For the base class, this is
accomplished using CodeGenFunction::EmitStaticVarDecl, and this can
be done in any appropriate way in a subclass.  If, for example, the
implementation uses a hidden pointer argument for __local variables,
it might emit getelementptr instructions indexing into the hidden
argument and insert those instructions into LocalDeclMap.

I'll write some of this up as comments for the next revision.

> 
> +// RUN: %clang_cc1 %s -ffake-address-space-map -emit-llvm -o - | FileCheck
> %s
> Why do you use the fake address map, not the CL one?

We need to be able to test that the variable i is in fact emitted
in the local address space.  The default address space map maps all
OpenCL address spaces to 0, so it would be impossible to test for
this with the default map.

> P.S. I nearly missed this patch of yours, I as normally only read cfe-dev.
> I appreciate, however, that sending patches to cfe-commits is more
> appropriate.  Could you please cc me on submissions to cfe-commits?

I can certainly do that, but not everyone on the list will know to
copy OpenCL related patches to you.  Perhaps you can subscribe to
cfe-commits and use a mail filter to filter out anything not OpenCL
related?

Thanks,
-- 
Peter