[PATCH] D57464: Generalize method overloading on addr spaces to C++

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: test/SemaCXX/address-space-method-overloading.cpp:28
+   //inas4.bar();
+   noas.bar(); // expected-error{{call to member function 'bar' is ambiguous}}
+}
----------------
ebevhan wrote:
> rjmccall wrote:
> > ebevhan wrote:
> > > Anastasia wrote:
> > > > ebevhan wrote:
> > > > > Just as an aside (I think I mentioned this on one of the earlier patches as well); treating a non-AS-qualified object as being in a 'generic' AS even for normal C++ isn't a great idea IMO. The object initialization code should be checking if the ASes of the actual object and the desired object type are compatible, and only if so, permit the overload.
> > > > I think changing this would require changing AS compatibility rules in general, not just for overloading but for example for conversions too. This seems like a wider scope change that might affect backwards compatibility. We might need to start an RFC on this first. John has suggested adding a target setting  for this on the other review. I think that should work.  Another idea could be to add some compiler directives to specify what generic address space is (if any).
> > > > 
> > > > Using default (no address space) as generic has a lot of advantages since it doesn't need many parser changes. In OpenCL we weren't lucky that initial implementation was added that used default for private memory and therefore when generic was introduced we had to map it to a new lang addr space value. That required a lot of changes in the parser. But once we fix those actually, anyone should be able  to map generic to anything. I initially thought it has no other use cases than in OpenCL but now I feel there might be a value to map default case (no address space) for something specific and then use generic to specify a placeholder address space on pointers or references. We could just expose generic address space generically and also have a mode with no generic address space. The latter means that conversions between different address spaces is never valid. Would this make sense?
> > > The big problem with the address space support in Clang is that it isn't really very well formalized unless you're on OpenCL. There's no real way for a target to express the relations between its address spaces; most of the relations that exist are hard-coded.
> > > 
> > > The support should probably be generalized for both targets and for LangASes like OpenCL's. Maybe:
> > > 
> > > * the ASes would be defined in a TableGen file which specifies which ASes exist, which ones are compatible/subsets/supersets, etc,
> > > * or, just have a target hook that lets a target express that a conversion from AS A to AS B is prohibited/permitted explicitly/permitted implicitly.
> > > 
> > > Just some loose ideas; an RFC for this is possibly the right way forward.
> > > 
> > > The reason I bring all of this up is primarily because in our target, the 'default' AS is disjoint from our other ones, so there's no 'generic' AS to be had. Both implicit and explicit conversion between these ASes is prohibited.  Since Clang currently doesn't allow you to express that ASes are truly incompatible, we have a flag that blocks such conversions unconditionally. Ideally it would be target-expressible.
> > > 
> > > -----
> > > 
> > > > I think changing this would require changing AS compatibility rules in general, not just for overloading but for example for conversions too. This seems like a wider scope change that might affect backwards compatibility. We might need to start an RFC on this first. John has suggested adding a target setting for this on the other review. I think that should work. Another idea could be to add some compiler directives to specify what generic address space is (if any).
> > > 
> > > I'm unsure whether any changes to the current semantics really need to be done, at least for the overloading problem.
> > > 
> > > For example, explicit conversion from a non-address space qualified pointer type to an address space qualified pointer type (and vice versa) is permitted, but implicit conversion is an error in both C and C++: https://godbolt.org/z/UhOC0g
> > > 
> > > For an overload candidate to be viable, there has to be an implicit conversion sequence for the implicit object argument to the implicit object parameter type. But no such implicit conversion sequence exists for types in different ASes, even today, or the implicit example in the godbolt would pass. So, an overload candidate with a different AS qualification than the object should not be viable.
> > > 
> > > This is clearly not compatible with OpenCL's notion of the `__generic` AS, but OpenCL already has logic for `__generic` in Qualifiers to handle that case (`isAddressSpaceSupersetOf`). Arguably, that behavior should probably not be hard coded, but that's how it is today.
> > > 
> > > (Also, just because an AS is a superset of another AS does not necessarily mean that the language should permit an implicit conversion between them, but it's a reasonable behavior, I guess.)
> > > 
> > > -----
> > > 
> > > Ultimately, whether or not a conversion of a pointer/reference from address space A to address space B is permitted should both be a function of what the target and the language allows, but that support doesn't exist. I also don't think that exposing a 'generic' AS is the right approach. There are ASes, and some conversions from ASes to other ASes are legal, while others are not. There's also the question of the difference between implicit and explicit conversions; blocking all implicit conversions between ASes would not work for OpenCL, so there must be a way to express that as well.
> > > 
> > > -----
> > > 
> > > These are all just loose thoughts from my head. I'm not terribly familiar with OpenCL except for what I've seen around the Clang code, so there might be details there that I'm not aware of.
> > > just have a target hook that lets a target express that a conversion from AS A to AS B is prohibited/permitted explicitly/permitted implicitly.
> > 
> > I think it has to be this, given the possibility of target-specific subspacing rules in numbered address spaces.  We can't make targets use disjoint sets of numbered address spaces.
> > 
> > I agree that, in general, the rule has to be that we check whether the object type is convertible to the expected address space of the method; we can't assume that there's a generic address space that works for everything.  That's not even true in OpenCL, since `constant` is not a subspace of the generic address space.
> > 
> > > John has suggested adding a target setting for this on the other review.
> > 
> > The suggestion is just that we recognize some way to control what the default address space for methods is.
> > 
> > This is difficult with member pointers because a function type can be turned into a member pointer type arbitrarily later; that is, we need to be able to canonically distinguish a function type that was written without any address space qualifier (and hence is `generic` if turned into a member pointer) from one that was written with our representationally-default address space qualifier (`private`).  We can deal with this later, I think, but it might require rethinking some things.
> > I think it has to be this, given the possibility of target-specific subspacing rules in numbered address spaces. We can't make targets use disjoint sets of numbered address spaces.
> 
> I'm in favor of this option as well.
> 
> > This is difficult with member pointers because a function type can be turned into a member pointer type arbitrarily later; that is, we need to be able to canonically distinguish a function type that was written without any address space qualifier (and hence is generic if turned into a member pointer) from one that was written with our representationally-default address space qualifier (private). We can deal with this later, I think, but it might require rethinking some things.
> 
> Oh, now I get the problem with those. That is tricky.
> 
> So when a function type is used as the type of a pointer-to-member and it is method-qualified with Default, it should be changed to be method-qualified with __generic instead?
If it isn't method-qualified at all, yes.  If that's representationally the same as being method-qualified with `private`, we have a problem — we can fix that in the short term by banning method qualification with `private`, but we do need a real fix.


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

https://reviews.llvm.org/D57464