[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

Tue May 7 07:44:58 PDT 2019

Anastasia marked an inline comment as done.
Anastasia added inline comments.

================
Comment at: test/CodeGenCXX/address-space-of-this.cpp:9
+//CHECK: call void @_ZN6MyTypeC1Ei(%struct.MyType* addrspacecast (%struct.MyType addrspace(10)* @m to %struct.MyType*), i32 123)
+MyType __attribute__((address_space(10))) m = 123;
----------------
rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Sorry I didn't catch this before, but I don't see why this test is expected to work.  We can't actually pass a pointer in address space 10 to that constructor.
> > Ok, I have created a bug to follow up on this issues:
> > https://bugs.llvm.org/show_bug.cgi?id=41730
> > 
> > It seems that the check is missing here for allowing the address space conversions implicitly, but I am afraid if I add it now addr spaces will become less usable because implicit conversions can't be setup by the target yet. And everyone is using no address space as some sort of `__generic` but unofficially. :(
> > 
> > I have some thoughts about adding something like `__generic` address space to C++ that can either be resolved by the compiler or supported by HW. I think it might help to implement those cases correctly without modifying too much of code base. I just struggled to find enough bandwidth to send an RFC but I will try to progress on this asap.
> > Ok, I have created a bug to follow up on this issues:
> 
> Thanks.
> 
> > It seems that the check is missing here for allowing the address space conversions implicitly, but I am afraid if I add it now addr spaces will become less usable because implicit conversions can't be setup by the target yet. And everyone is using no address space as some sort of __generic but unofficially. :(
> 
> As far as I'm concerned, address-space support in the C++ feature set is all still extremely experimental and there are no real users that we have to worry about making things less useful for.  The right thing for the basic model is for constructors to only work when the object is being constructed in an address space that's convertible to the address space of the constructor.  Languages with a `__generic` superspace (whether implemented with dynamic selection or cloning or anything else) can consider making it the default address space of constructors, but that's not something we should be pushing in the basic model.
> As far as I'm concerned, address-space support in the C++ feature set is all still extremely experimental and there are no real users that we have to worry about making things less useful for. 

Ok, then I can try to fix that generically. And at least we can test it using `__constant` in OpenCL.

> The right thing for the basic model is for constructors to only work when the object is being constructed in an address space that's convertible to the address space of the constructor.

Just to understand a bit more. Would the constructor address space be given in the source code or would it be up to the target to set it up? Also would it be applied to all other methods too?

> Languages with a __generic superspace (whether implemented with dynamic selection or cloning or anything else) can consider making it the default address space of constructors, but that's not something we should be pushing in the basic model.

Ok, I have drafted an RFC around this topic that I was planning to share with Clang dev community at some point.
https://docs.google.com/document/d/1YybeeRgGrckMjxjtLdRUf0V5f9Psq97cWxBN4npDoqk/edit?usp=sharing

Just in short the idea of this proposal is to make something like `__generic` address space as a feature configurable by the targets. It is extending the original idea described in http://lists.llvm.org/pipermail/cfe-dev/2019-March/061541.html

I am also discussing briefly some extra ideas of language based solutions to provide information on address spaces for class instantiations at the source level. Your feedback would be highly appreciated even at this very early stage (of course if you have any extra bandwidth). It would be much easier to fix remaining issues once we agree on the future directions. Thanks!

Repository:
  rC Clang

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

https://reviews.llvm.org/D59988