Anastasia marked an inline comment as done.
Anastasia added a comment.
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?
> The right place to write the qualifier is the same place where you have to
> write attributes and/or `mutable`, i.e. in the lambda declarator, something
> like `[]() __local { ... }`.
>
> Arguably there ought to be a special-case rule allowing lambdas with no
> captures to be called in an arbitrary address space, but I don't think we
> need to go there in this patch.
I am trying to understand what are the limitations here. Once we add ability to
qualify lambdas with an addr space it should just work? But also it should be
ok for all lambdas? Apart from captures won'twork for `__constant` addr space
in OpenCL.
To summarize what has to be done overall to support addr spaces with lambdas:
1. Add default AS on object param of internal lambda class member functions.
2. Allow addr space qualifier on lambda expression.
Example:
[&]() __local {i++;};
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.
================
Comment at: clang/test/SemaOpenCLCXX/address-space-lambda.cl:13
+ __constant auto err = [&]() {}; //expected-note-re{{candidate function not
viable: address space mismatch in 'this' argument ('__constant (lambda at
{{.*}})'), parameter type must be 'const __generic (lambda at {{.*}})'}}
+ err(); //expected-error-re{{no matching function for call to object of type
'__constant (lambda at {{.*}})'}}
+}
----------------
rjmccall wrote:
> Can we get a better diagnostic here when a candidate would have succeeded
> except for an address-space mismatch? Something like "candidate function not
> viable: cannot convert 'this' from '__constant' address space to
> '__generic'"? You can do that in a separate patch, of course.
Sure. Makes sense!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69938/new/
https://reviews.llvm.org/D69938
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
