Anastasia added a comment. In D69938#1745766 <https://reviews.llvm.org/D69938#1745766>, @rjmccall wrote:
> 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 can't think of any other way. The main reason is that in OpenCL we add address spaces implicitly to all objects if they are not specified. Unless we decide to modify `decltype` but I think in other places it is useful to copy addr space qualifier too. For C++ however it might work, I will look into it. > 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(); > I was already planning to add this. I could look into it next and maybe just a add FIXME in the test for now. >> 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. Btw global lambda objects are in `__global` address space in OpenCL. 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
