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

Mon May 20 11:32:44 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:
> > > 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!
> > 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?
> 
> 1. It should be possible to give a method / constructor an address space explicitly in source code.  This would be treated basically like a `const` or `volatile` qualifier on the method would in terms of overloading, mangling, etc.  To support this properly for constructors will require some refactoring in Sema to propagate the address space of the object we're constructing into the initialization / overload-resolution code — usually that'll be `LangAS::Default`, but there are other contexts where we can pick up other address spaces, like if we're initializing a global (and the target knows how to allocate in that address space.
> 
> 1a. As a corollary, we should allow `operator new` to be address-space qualified, which is weirder (because it's not a non-static member function) but I think the only reasonable way to manage that.
> 
> 2. I think it's reasonable to have language modes and targets be able to set a default address space for methods and constructors that isn't necessarily `LangAS::Default`.  Note that this implies that there needs to be some way to spell `LangAS::Default` on the target, though, since it's almost surely important to be able to declare methods there.
> 
> At some point I'm not sure that cfe-dev is even the right place for this; you might want to write up some sort of report to the C++ committee about recommendations for extending C++ to generalized address spaces, in the interest of creating a sort of Embedded C++ spec.
>     It should be possible to give a method / constructor an address space explicitly in source code. This would be treated basically like a const or volatile qualifier on the method would in terms of overloading, mangling, etc. To support this properly for constructors will require some refactoring in Sema to propagate the address space of the object we're constructing into the initialization / overload-resolution code — usually that'll be LangAS::Default, but there are other contexts where we can pick up other address spaces, like if we're initializing a global (and the target knows how to allocate in that address space.
> 
> 1a. As a corollary, we should allow operator new to be address-space qualified, which is weirder (because it's not a non-static member function) but I think the only reasonable way to manage that.
> 
>     I think it's reasonable to have language modes and targets be able to set a default address space for methods and constructors that isn't necessarily LangAS::Default. Note that this implies that there needs to be some way to spell LangAS::Default on the target, though, since it's almost surely important to be able to declare methods there.

Ok, that seems to align with what I was thinking to implement. Btw, I have a fix to prevent incorrect addr space cast while constructing objects that covers this test: https://reviews.llvm.org/D62156

> 
> At some point I'm not sure that cfe-dev is even the right place for this; you might want to write up some sort of report to the C++ committee about recommendations for extending C++ to generalized address spaces, in the interest of creating a sort of Embedded C++ spec.

I agree this should be the end goal. However, I feel a lot more confident to write the proposal if I have a working tool. Also I would like to leverage the diverse knowledge in Clang community to make sure the ideas cover variety of different architecture and use cases. I believe it might take some time until we get something more concrete in the spec and having some sort of working tool would allow us to provide temporary base for some solutions.

Repository:
  rC Clang

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

https://reviews.llvm.org/D59988