Page MenuHomePhabricator

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

Authored by Anastasia on Mar 29 2019, 4:48 AM.

Details

Summary

The example from the bugzilla triggered two issues:

  1. In qualification conversion we are creating an address space conversion for non-pointer and non-reference type.
  2. We are not taking an address space from the right type when attempting to cast argument of 'this' to its parameter type.

Diff Detail

Repository
rC Clang

Event Timeline

Anastasia created this revision.Mar 29 2019, 4:48 AM
Anastasia marked an inline comment as done.Mar 29 2019, 4:53 AM
Anastasia added inline comments.
lib/CodeGen/CGClass.cpp
2039

I am a bit unsure if performAddrSpaceCast should be used, but considering that we know that we are not casting a constant it should be fine?

If not any suggestions how to propagate LangAS of 'this' here. Some thoughts I have are:

  • Try to list the conversion up in the call stack
  • Pass LangAS all the way to here
rjmccall added inline comments.Mar 29 2019, 9:11 PM
lib/CodeGen/CGClass.cpp
2039

I feel like This should just be in the right address space for the constructor at the point EmitCXXConstructorCall is called. We don't expect this function to do any other semantic conversions. Or is this necessary to handle special-case use of things like trivial default / copy constructors?

lib/Sema/SemaInit.cpp
7324

If this function is being used to do pointee qualification conversions on pointer r-values, I think you need to look at whether the pointee address spaces are different, right?

brunodf added a subscriber: brunodf.Apr 1 2019, 1:35 AM
brunodf added inline comments.
lib/CodeGen/CGClass.cpp
2039

Where could the conversion of this be listed in the clang AST? this seems implicit there.

Passing along LangAS seems to have some precedent. EmitCXXConstructExpr (which calls EmitCXXConstructorCall) works on AggValueSlot which carries the original qualifiers. Currently not yet used for address space (but this seems similar to me):

/// \param quals - The qualifiers that dictate how the slot should
/// be initialied. Only 'volatile' and the Objective-C lifetime
/// qualifiers matter.
rjmccall added inline comments.Apr 1 2019, 10:03 AM
lib/CodeGen/CGClass.cpp
2039

Passing down the address space also works, whether in an AggValueSlot or an LValue or whatever, and I suppose it's better for the special cases for trivial constructors.

Anastasia updated this revision to Diff 193281.Apr 2 2019, 7:09 AM
  • Use AggValueSlot in the constructor call generation to store/retrieve address space of 'this'.
  • Fixed detecting the address space conversion while performing qualification conversion.
rjmccall accepted this revision.Apr 3 2019, 6:57 AM

LGTM.

This revision is now accepted and ready to land.Apr 3 2019, 6:57 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 3:49 AM
rjmccall added inline comments.Apr 10 2019, 10:06 AM
test/CodeGenCXX/address-space-of-this.cpp
9

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.

Anastasia marked an inline comment as done.May 3 2019, 7:46 AM
Anastasia added inline comments.
test/CodeGenCXX/address-space-of-this.cpp
9

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.

kpet added a subscriber: kpet.May 3 2019, 8:06 AM
rjmccall added inline comments.May 3 2019, 12:59 PM
test/CodeGenCXX/address-space-of-this.cpp
9

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.

Anastasia marked an inline comment as done.May 7 2019, 7:43 AM
Anastasia added inline comments.
test/CodeGenCXX/address-space-of-this.cpp
9

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!

rjmccall added inline comments.May 7 2019, 10:48 AM
test/CodeGenCXX/address-space-of-this.cpp
9

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.

  1. 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.

Anastasia marked an inline comment as done.May 20 2019, 11:29 AM
Anastasia added inline comments.
test/CodeGenCXX/address-space-of-this.cpp
9
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.