[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D69938#1745737 <https://reviews.llvm.org/D69938#1745737>, @Anastasia wrote:

> In D69938#1742354 <https://reviews.llvm.org/D69938#1742354>, @rjmccall wrote:
>
> > In D69938#1742026 <https://reviews.llvm.org/D69938#1742026>, @Anastasia wrote:
> >
> > > In D69938#1741713 <https://reviews.llvm.org/D69938#1741713>, @rjmccall wrote:
> > >
> > > > In D69938#1741080 <https://reviews.llvm.org/D69938#1741080>, @Anastasia wrote:
> > > >
> > > > > In D69938#1737196 <https://reviews.llvm.org/D69938#1737196>, @rjmccall wrote:
> > > > >
> > > > > > It does make logical sense to be able to qualify the call operator of a lambda.  The lambda always starts as a temporary, so presumably we want the default address space qualifier on lambdas to be compatible with the address space of temporaries.  However, that's probably also true of the default address qualifier on methods in general or you wouldn't be able to call them on temporaries and locals.  Thus, we should be able to use the same default in both cases, which seems to be what you've implemented.
> > > > > >
> > > > > > It even makes sense to be able to qualify a lambda with an address space that's not compatible with the address space of temporaries; that just means that the lambda has to be copied elsewhere before it can be invoked.
> > > > >
> > > > >
> > > > > Just to clarify... Do you mean we should be able to compile this example:
> > > > >
> > > > >   [&] __global {
> > > > >         i++;
> > > > >   } ();
> > > > >   
> > > > >   
> > > > >
> > > > > Or should this result in a diagnostic about an addr space mismatch?
> > > >
> > > >
> > > > It should result in a diagnostic on the call.  But if you assigned that lambda into global memory (somehow) it should work.
> > >
> > >
> > > Ok, makes sense so something like this should compile fine:
> > >
> > >   __global auto glob = [&] __global { ... };
> >
> >
> > Right, a call to `glob()` should work in this case.  I don't know if `__global` would parse here without `()`, though; in the grammar, it looks like attributes and so on have to follow an explicit parameter clause.
> >
> > >>> 3. Diagnose address space mismatch between variable decl and lambda expr qualifier Example: __private auto  llambda = [&]() __local {i++;}; // error no legal conversion b/w __private and __local
> > >>> 
> > >>>   I think 1 is covered by this patch. I would like to implement 2 as a separate patch though to be able to commit fix for 1 earlier and unblock some developers waiting for the fix. I think 3 already works and I will just update the diagnostic in a separate patch too.
> > >> 
> > >> I agree that 3 should just fall out.  And yeah, implementing 2 as a separate patch should be fine.  Please make sure 3 is adequately tested in this patch.
> > > 
> > > Ok, since we can't qualify lambda expr with addr spaces yet there is not much I can test at this point. `__constant` lambda variable seems to be the only case with a diagnostic for OpenCL.
> >
> > You can certainly copy a lambda type into a different address space, or construct a pointer to a qualified lambda type, e.g. `(*(__constant decltype(somelambda) *) nullptr)()`.
>
>
> Perhaps I am not thinking of the right use cases, but I am struggling to understand what would be the difference to variables of other type here.
>
> I have added `(*(__constant decltype(somelambda) *) nullptr)()` as a test case however. It is rejected due to multiple addr spaces.


Hmm.  That makes sense — `decltype` is picking up the address-space qualification of `somelambda` — but seems unfortunate; is the only way to change the address space of a type to specifically strip the address space with a template metaprogram?  I would hope that the multiple-address-spaces error would only apply in the case of multiple explicit qualifiers, and otherwise the new address space qualifier would replace the old one.  I won't ask you to do that just to do this patch, but you could write the template metaprogram:

  template <class T> struct remove_address_space { using type = T; };
  template <class T> struct remove_address_space<__private T> { using type = T; };
  ...
  auto globalLambdaPtr = (__global remove_address_space<decltype(somelambda)>::type*) nullptr;
  globalLambdaPtr();



> Also generally do you think there is any situation in which having lambda object and lambda call operator in different addr spaces would be useful?

Absolutely; remember that all lambda objects begin their life as temporaries and are thus in the private address space, so if you want the lambda to be storable in any other address space you will have at least a brief mismatch.  The user can address-space-qualify the lambda all they want, but we can't generally automatically allocate memory for it that address space.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69938/new/

https://reviews.llvm.org/D69938