Add support for target-configurable address spaces.

[ebevhan]

Summary

This patch adds support in Clang for targets to configure
the relations between address spaces, making it possible
for a target to express which address space conversions
are permitted/invalid.

The original RFC is here: http://lists.llvm.org/pipermail/cfe-dev/2019-March/061541.html
However, the design in this patch has deviated from the
original suggestion.

It does multiple things to accomplish its goal:

• Moves QualType/Qualifiers accessors which deal with qualifier relations (such as compatiblyIncludes, etc.) to ASTContext, as Qualifiers cannot be made aware of the relations between address spaces on their own.
• Adds APIs to TargetInfo for querying address space relationships:
  • isAddressSpaceSuperSetOf is used to ask a target whether an address space is a superspace of another. Implicit conversion from a subspace to a superspace is always allowed, but not the other way around. By default, all address spaces are disjoint.
  • isExplicitAddrSpaceConversionLegal is used to ask whether an explicit conversion (e.g. via a cast) is permitted regardless of the address space relations given by the first method. By default, all explicit casts are allowed.

The ASTContext accessors will obey rules for LangASes first,
and then proceed to ask the target.

The reason for adding isExplicitAddrSpaceConversionLegal
is that according to Embedded-C, explicit casting between
all address spaces is permitted, but is undefined behavior
if the address spaces do not overlap.

This default behavior is odd, because there's no reason to
permit AS casts that will always be UB. However, as this has
been the default behavior since ASes were introduced, adding
this interface was the easiest way to keep supporting the
default behavior while also permitting targets to disallow
AS conversions that they do not want to allow.

There are still a few suspected issues with how explicit
casting is handled; in some cases, we do not know whether
we are performing an explicit cast, so we cannot know if
we should ask isExplicitAddrSpaceConversionLegal or
isAddressSpaceSuperSetOf.

[Anastasia]

This patch does not address the issue with the accessors
on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
because I don't know how to solve it without breaking a ton of
rather nice encapsulation. Either, every mention of compatiblyIncludes
must be replaced with a call to a corresponding ASTContext method,
Qualifiers must have a handle to ASTContext, or ASTContext must be
passed to every such call. This revision mentions the issue in a comment:
https://reviews.llvm.org/D49294

I think using ASTContext helper is ok then.

I was just thinking about testing the new logic. Should we add something like -ffake-address-space-map (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces.

lib/Sema/SemaOverload.cpp

3171

I guess if address spaces don't overlap we don't have a valid qualification conversion. This seems to align with the logic for cv. My guess is that none of the other conversions will be valid for non overlapping address spaces and the error will occur.

I think at this point we might not need to know if it's implicit or explicit? I believe we might have a separate check for this somewhere because it works for OpenCL. I don't know though if it might simplify the flow if we move this logic rather here.

The cv checks above seem to use CStyle flag. I am wondering if we could use it to detect implicit or explicit. Because we can only cast address space with C style cast at the moment. Although after adding addrspace_cast operator that will no longer be the only way.

5150

Agree! We need to check that address spaces are not identical and then validity!

But I guess it's ok for C++ because we can't add addr space qualifier to implicit object parameter yet?

It's broken for OpenCL though!

[ebevhan]

In D62574#1523835, @Anastasia wrote:

This patch does not address the issue with the accessors
on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
because I don't know how to solve it without breaking a ton of
rather nice encapsulation. Either, every mention of compatiblyIncludes
must be replaced with a call to a corresponding ASTContext method,
Qualifiers must have a handle to ASTContext, or ASTContext must be
passed to every such call. This revision mentions the issue in a comment:
https://reviews.llvm.org/D49294

I think using ASTContext helper is ok then.

Alright. Just to clarify, you mean the first option (using a new accessor on ASTContext and replacing all existing compatiblyIncludes with that)?

I'll have to see if that's possible without breaking a few more interfaces, since you can throw around Qualifiers and check for compatibility without an ASTContext today.

I was just thinking about testing the new logic. Should we add something like -ffake-address-space-map (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces.

Well, there are a couple targets which have target address spaces even today, I think. AMDGPU should be one, right?

I forgot to mention; this patch also disables two "extensions" that Clang has, namely that subtraction and comparison on pointers of different address spaces is legal regardless of address space compatibility. I'm pretty sure that these extensions only exist because Clang hasn't had support for targets to express address space compatibility until now. According to the docs, x86 uses address spaces for certain types of segment access: https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments

If x86 (or any target that uses target address spaces) wants to keep using this and still keep such pointers implicitly compatible with each other, it will need to configure this in its target hooks.

lib/Sema/SemaOverload.cpp

3171

I guess if address spaces don't overlap we don't have a valid qualification conversion. This seems to align with the logic for cv. My guess is that none of the other conversions will be valid for non overlapping address spaces and the error will occur.

Right. So the reasoning is that if the address spaces are disjoint according to the overlap rule, then it cannot be considered a qualification conversion.

But with the new hooks, it's possible for a target to allow explicit conversion even if address spaces do not overlap according to the rules. So even though there is no overlap, such a conversion could still be a qualification conversion if it was explicit (either via a c-style cast or an addrspace_cast). This is in fact the default for all targets (see the patch in TargetInfo.h).

I think I need a refresher on how the casts were meant to work; were both static_cast and reinterpret_cast supposed to be capable of implicit conversion (say, private -> generic) but only addrspace_cast for the other direction (generic -> private)? Or was reinterpret_cast supposed to fail in the first case?

5150

But I guess it's ok for C++ because we can't add addr space qualifier to implicit object parameter yet?

True, but that will become possible eventually.

I'm not sure if this is currently an issue with OpenCL as it is either. It's only a problem if FromType is not qualified, but is that possible? Even the 'notational' non-AS-qualification in OpenCL is still a qualification under the hood (either private or generic). Or maybe I'm misunderstanding how it works.

[Anastasia]

In D62574#1527126, @ebevhan wrote:

In D62574#1523835, @Anastasia wrote:

This patch does not address the issue with the accessors
on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
because I don't know how to solve it without breaking a ton of
rather nice encapsulation. Either, every mention of compatiblyIncludes
must be replaced with a call to a corresponding ASTContext method,
Qualifiers must have a handle to ASTContext, or ASTContext must be
passed to every such call. This revision mentions the issue in a comment:
https://reviews.llvm.org/D49294

I think using ASTContext helper is ok then.

Alright. Just to clarify, you mean the first option (using a new accessor on ASTContext and replacing all existing compatiblyIncludes with that)?

Yes, it seems ok to me. Not sure if @rjmccall has an opinion.

I'll have to see if that's possible without breaking a few more interfaces, since you can throw around Qualifiers and check for compatibility without an ASTContext today.

I was just thinking about testing the new logic. Should we add something like -ffake-address-space-map (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces.

Well, there are a couple targets which have target address spaces even today, I think. AMDGPU should be one, right?

Yes, however I am not sure we will be able to test more than what we test with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete idea of what to test?

I forgot to mention; this patch also disables two "extensions" that Clang has, namely that subtraction and comparison on pointers of different address spaces is legal regardless of address space compatibility. I'm pretty sure that these extensions only exist because Clang hasn't had support for targets to express address space compatibility until now. According to the docs, x86 uses address spaces for certain types of segment access: https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments

If x86 (or any target that uses target address spaces) wants to keep using this and still keep such pointers implicitly compatible with each other, it will need to configure this in its target hooks.

Ok, it seems logical to me. In OpenCL we don't allow those cases.

lib/Sema/SemaOverload.cpp

3171

Just as a summary:

• All casts should allow safe/implicit AS conversions i.e. __private/__local/__global -> __generic in OpenCL
• All casts except for C-style and addrspace_cast should disallow unsafe/explicit conversions i.e. generic -> __private/__local/__global in OpenCL
• All casts should disallow forbidden conversions with no address space overlap i.e. __constant <-> any other in OpenCL

In OpenCL overlapping logic is only used for explicit i.e. unsafe conversion. So it seems we might only need those conversions here then?

5150

Yes, ideally we deduce address spaces if they are not specified explicitly. However I am not sure this is currently the case everywhere and whether we can have weird interplay with templates logic. One case I have in mind is function return type that is kept with Default address space but it shouldn't ever occur with the logic here.

So perhaps theoretically it's not broken for OpenCL. :)

[ebevhan]

I'll have to see if that's possible without breaking a few more interfaces, since you can throw around Qualifiers and check for compatibility without an ASTContext today.

I was just thinking about testing the new logic. Should we add something like -ffake-address-space-map (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces.

Well, there are a couple targets which have target address spaces even today, I think. AMDGPU should be one, right?

Yes, however I am not sure we will be able to test more than what we test with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete idea of what to test?

Well, since there is a mapping from the OpenCL address spaces to the AMDGPU target spaces, I would presume that if the target spaces were used on their own (via __attribute__((address_space(1))) for example) they should behave similarly to their OpenCL counterparts. It wouldn't have to be much, just a couple of type definitions and checks for implicit/explicit cast behavior.

There's also the x86 case I mentioned. I'm not really sure how that feature is used, though.

[ebevhan]

Replaced compatiblyIncludes and its dependents with ASTContext accessors instead.

[Anastasia]

In D62574#1534159, @ebevhan wrote:

I'll have to see if that's possible without breaking a few more interfaces, since you can throw around Qualifiers and check for compatibility without an ASTContext today.

I was just thinking about testing the new logic. Should we add something like -ffake-address-space-map (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces.

Well, there are a couple targets which have target address spaces even today, I think. AMDGPU should be one, right?

Yes, however I am not sure we will be able to test more than what we test with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete idea of what to test?

Well, since there is a mapping from the OpenCL address spaces to the AMDGPU target spaces, I would presume that if the target spaces were used on their own (via __attribute__((address_space(1))) for example) they should behave similarly to their OpenCL counterparts. It wouldn't have to be much, just a couple of type definitions and checks for implicit/explicit cast behavior.

There's also the x86 case I mentioned. I'm not really sure how that feature is used, though.

Ok, I think at some point you might want to test extra functionality that doesn't fit into OpenCL model, for example explicit conversion over non-overlapping address spaces? I think for this you might find useful to add the extra flag that switches on extra testing with "fake" setup of address spaces.

include/clang/AST/ASTContext.h

2555

Is there any advantage of keeping superset&subset concept? Amd if yes, how do we position the new functionality with explicit cast?

I think I am missing a bit conceptual view... because I think we originally discussed to switch to implicit/explicit conversion model. Perhaps there is no reason to do it but I would just like to understand why?

lib/Sema/SemaCast.cpp

2223

It seems like you are changing the functionality here. Don't we need any test for this?

2318

Btw I think this is set in:
https://reviews.llvm.org/D62299

Although I don't have any objections to changing to this.

2334

I think you might need to update the comment above to reflect that you are generalizing the behavior.

[ebevhan]

In D62574#1552220, @Anastasia wrote:

Ok, I think at some point you might want to test extra functionality that doesn't fit into OpenCL model, for example explicit conversion over non-overlapping address spaces? I think for this you might find useful to add the extra flag that switches on extra testing with "fake" setup of address spaces.

That functionality is actually enabled by default to allow for Clang's current semantics. Embedded-C allows all explicit conversions by default, and OpenCL in Clang allows all explicit conversion to and from target spaces as an extension.

It's rather up to each target to *disable* the default explicit conversion behavior if they don't want it.

include/clang/AST/ASTContext.h

2555

Yes, I know the original discussion was regarding the implicit/explicit model, but I came to realize during the implementation that all that was needed to get the superspace model to work generically with the current behavior was an override on the explicit conversion.

The implicit/explicit model also has the drawback that it's a bit too expressive. You can express semantics that just don't really make sense, like permitting implicit conversion but not explicit conversion. The superspace model doesn't let you do that, and the additions I've made here still don't: If implicit conversion is allowed, explicit conversion must also be allowed. It just becomes possible to allow explicit conversion for ASes that don't overlap.

Since the superspace model is what OpenCL and Embedded-C use in their specification, it's probably better to use that anyway.

lib/Sema/SemaCast.cpp

2223

I don't think it's necessarily changing. If we are doing a reinterpret_cast that stems from a c-style cast, we want to check that explicit conversion is allowed. This happens both if either AS overlaps, or if the target permits it. If it's not a C-style cast, then we need to check for subspace correctness instead, as reinterpret_cast can only do 'safe' casts.

The original code here allows all explicit C-style casts regardless of AS, but now we have to ask the target first.

2318

Oh, indeed.

[ebevhan]

Rebased and addressed some comments.

[Anastasia]

include/clang/AST/ASTContext.h

2555

The implicit/explicit model also has the drawback that it's a bit too expressive. You can express semantics that just don't really make sense, like permitting implicit conversion but not explicit conversion. The superspace model doesn't let you do that, and the additions I've made here still don't: If implicit conversion is allowed, explicit conversion must also be allowed. It just becomes possible to allow explicit conversion for ASes that don't overlap.

Ok, I think we could define the new model something like - explicit conversions are automatically allowed for all implicit conversions... targets won't have to specify those but only extra comversions that are not allowed implicitly.

Just to understand in the current patch when are we supposed to use isAddressSpaceOverlapping and when isExplicitAddrSpaceConversionLegal. Can't we just always use isExplicitAddrSpaceConversionLegal?

Since the superspace model is what OpenCL and Embedded-C use in their specification, it's probably better to use that anyway.

I agree the advantage of following spec is really huge. However, Clang is already broken for Emdedded C isn't it? Because it allows any explicit conversions?

As for OpenCL it might be reasonable to provide new documentation if needed as soon as the new rules don't invalidate all behavior.

lib/Sema/SemaCast.cpp

2223

Ok, but shouldn't we test the new functionality of rejecting C style casts if target doesn't permit the conversion?

Also I believe C style cast functionality is being handled in TryAddressSpaceCast, and it probably belongs there. In fact you have already extended this:

if (!DestPtrType->isAddressSpaceOverlapping(*SrcPtrType)) {
    msg = diag::err_bad_cxx_cast_addr_space_mismatch;
    return TC_Failed;
}

Why is this change here needed? What test case does it cover?

2326

I would keep the OpenCL part but move it to the end as an example. It often helps when there are spec references in the code.

// OpenCL v2.0 s6.5.5 - Generic addr space overlaps with any named one, except for constant.

lib/Sema/SemaExpr.cpp

10589–10591

Should this also be allowed if pointers are explicitly convertible?

lib/Sema/SemaOverload.cpp

3171

Did you have time to look into this?

[ebevhan]

Sorry for the very late response to this!

include/clang/AST/ASTContext.h

2555

Ok, I think we could define the new model something like - explicit conversions are automatically allowed for all implicit conversions... targets won't have to specify those but only extra comversions that are not allowed implicitly.

Yes, this is how it's defined. Converting explicitly between two ASes where either one is a superset of the other is always legal.

Just to understand in the current patch when are we supposed to use isAddressSpaceOverlapping and when isExplicitAddrSpaceConversionLegal. Can't we just always use isExplicitAddrSpaceConversionLegal?

I guess the distinction between isAddressSpaceOverlapping and isExplicitAddrSpaceConversionLegal is pretty subtle. You would want the former when you need to know if implicit conversion A->B or B->A is permitted, and the latter when you need to know if explicit conversion A->B is permitted.

Though in most cases, I think the latter is probably the most common.

I agree the advantage of following spec is really huge. However, Clang is already broken for Emdedded C isn't it? Because it allows any explicit conversions?

No, the current behavior of permitting all explicit conversions is according to spec: "A non-null pointer into an address space A can be cast to a pointer into another address space B, but such a cast is undefined if the source pointer does not point to a location in B." The addition of isExplicitAddrSpaceConversionLegal lets targets override this behavior and make such casts non-legal.

lib/Sema/SemaCast.cpp

2223

Ok, but shouldn't we test the new functionality of rejecting C style casts if target doesn't permit the conversion?

Absolutely, but there are no targets which use the functionality yet, and I wouldn't want to make any such change without consulting with a target owner first.

Also I believe C style cast functionality is being handled in TryAddressSpaceCast, and it probably belongs there. Why is this change here needed? What test case does it cover?

That's a good point! Address space conversion for c-style casts should have already been handled by the time we get to TryReinterpretCast. So doesn't that mean that this code path is dead in trunk as well?

lib/Sema/SemaExpr.cpp

10589–10591

No, I don't think so. We have some kind of AS_A < AS_B here, and if we check if either the LHS or the RHS can be implicitly converted to the other, we are safe. Checking explicit conversion isn't interesting here, because there is no explicit conversion in the code.

This is one of the cases I mentioned in the other comment about wanting to know if either AS can be implicitly converted, rather than knowing if one can be explicitly converted to the other.

lib/Sema/SemaOverload.cpp

3171

I still don't really understand why the code checks for overlap here. Removing this check causes one test case to break, CodeGenCXX/address-space-cast.cpp. Specifically, this:

#define __private__ __attribute__((address_space(5)))

void test_cast(char *gen_char_ptr, void *gen_void_ptr, int *gen_int_ptr) {
  __private__ void *priv_void_ptr = (__private__ void *)gen_char_ptr;
}

It tries to resolve the C-style cast with TryAddressSpaceCast, but fails as the underlying pointee types (char and void) are different. Then it tries to do it as a static_cast instead, but fails to produce an AddressSpaceConversion; instead, it makes a BitCast as the second conversion step which causes CodeGen to break since the conversion kind is wrong (the address spaces don't match).

Is address space conversion supposed to be a pointer conversion or a qualification conversion? If the second conversion step had emitted an AddressSpaceConversion instead of a BitCast, it would have worked. But at the same time, if we have IsQualificationConversion return false whenever we would need to do an address space conversion, other tests break.

I suppose that a solution might be to remove the special case in IsQualificationConversion for address spaces, and that TryAddressSpaceCast should ignore the underlying type if it's part of a C-style cast. That way we won't try to handle it as a static_cast. But I don't know if that's just a workaround for a larger underlying issue, or if it causes other issues.

All in all, I have no idea what these code paths are trying to accomplish. It just doesn't make sense to me. :(

[Anastasia]

include/clang/AST/ASTContext.h

2555

I see so we are making the new rules then. I guess it should be fine. However we might want to document this better and explain the difference between two types of API provided.

lib/Sema/SemaCast.cpp

2223

Absolutely, but there are no targets which use the functionality yet, and I wouldn't want to make any such change without consulting with a target owner first.

Alternatively we can add a flag just for testing to override the address space settings for targets. Or may be we could just reuse -ffake-address-space-map. I believe I've mentioned it before.

That's a good point! Address space conversion for c-style casts should have already been handled by the time we get to TryReinterpretCast. So doesn't that mean that this code path is dead in trunk as well?

Yeah, possibly we have overlooked that.

lib/Sema/SemaOverload.cpp

3171

It tries to resolve the C-style cast with TryAddressSpaceCast, but fails as the underlying pointee types (char and void) are different. Then it tries to do it as a static_cast instead, but fails to produce an AddressSpaceConversion; instead, it makes a BitCast as the second conversion step which causes CodeGen to break since the conversion kind is wrong (the address spaces don't match).

Strange! TryStaticCast should set cast kind to CK_AddressSpaceConversion if it detects the address spaces are different. Do you know why it doesn't happen for this test case?

Is address space conversion supposed to be a pointer conversion or a qualification conversion?

It is a qualification conversion.

I suppose that a solution might be to remove the special case in IsQualificationConversion for address spaces, and that TryAddressSpaceCast should ignore the underlying type if it's part of a C-style cast. That way we won't try to handle it as a static_cast. But I don't know if that's just a workaround for a larger underlying issue, or if it causes other issues.

I think the idea of separate conversions is for each of them to work as a separate step. So address space cast should be working just as const cast i.e. it should only check the address space conversion and not the type. However the problem here is that addrspacecast supersedes bitcast. Therefore there are extra checks in the casts later to classify the cast kind correctly. If this flow fails we can reevaluate but this is the idea we were following up to now.

[ebevhan]

Sorry for the very late response on this. Hope it's not completely off your radar.

include/clang/AST/ASTContext.h

2555

I will add some more comments to the functions to describe a bit better what they should be used for.

lib/Sema/SemaCast.cpp

2223

I haven't looked more into the testing issue yet.

---

I think that reinterpret_casts for c-style casts must handle AS conversions like this as it must be possible to do a conversion like AS1 T1 * -> AS2 T2 * where:

• T1 is not a derived of T2
• T2 is not void

and AS2 is not a superspace of AS1.

In such cases neither addrspace_cast nor static_cast apply, so reinterpret_cast must be able to handle explicit AS conversions.

lib/Sema/SemaOverload.cpp

3171

I've determined that something doesn't add up with how conversions are generated. I'm pretty sure that the issue lies in PerformImplicitConversion.

When we analyze the c-style cast in this:

char * p;
__private__ void * pp = (__private__ void *)gen_char_ptr;

we determine that this cannot be an addrspace_cast (as the unqualified pointee types are different). It can be a static_cast, though. The implicit conversion resulting from this static_cast should occur in two steps: the second conversion should be a pointer conversion from char* to void*, and the third conversion should be a qualification conversion from void* to __private__ void*.

Now, this *is* the ICS/SCS that we get. However, when we realize the conversion sequence in PerformImplicitConversion, it actually completely ignores the intermediate types in the conversion sequence. Everything it does is only on the intermediate 'From' type after each conversion step and the final 'To' type, but we never consider the intermediate 'To' type. This means that it will try to realize the entire conversion sequence (both the 'bitcast' conversion and the 'addrspace' conversion) in a single (pointer conversion) step instead of two, and since CheckPointerConversion has no provisions for address space casts (because why should it?) we get a CK_BitCast and codegen fails since it's trying to change the address space of the type.

This means that each conversion step realization needs to be aware of every kind of CastKind that could possibly occur in each step, which is bizarre. We know exactly what type we are converting from and to in each conversion in the sequence; why do we use the *final* result type to determine what kind of implicit cast we should use in each step?

I've tried to make PerformImplicitConversion aware of the intermediate types, but this breaks a bunch of tests, as there are lots of checks and diagnoses in the function that require the final type. One contributor to this is that the 'ToType' of the SCS (ToType[2]) doesn't actually have to match the final 'ToType' of the conversion. Something to do with exception specifications.

It feels like this code is just broken and everyone has just been coding around the issues. Perhaps this is the root:

// Overall FIXME: we are recomputing too many types here and doing far too
// much extra work. What this means is that we need to keep track of more
// information that is computed when we try the implicit conversion initially,
// so that we don't need to recompute anything here.

I don't know what the solution here is, short of redesigning PerformImplicitConversion.

[danilaml]

What are the remaining roadblocks left before this patch can be merged? I'm interested in having a target-specific way to define the allowed explicit/implicit address space conversions.
Also, it appears that currently whether implicit casts between pointers of different AS are allowed is determined by the superset relations only, however https://reviews.llvm.org/D73360 introduced another (openCL-specific) variable into the mix: whether the conversion is a top level or not. IMHO, this should also be configured by the target, although I'm not sure whether it needs a separate hook (isBitcastNoop or similar?) or it needs to check the legality of the implicit casts differently.

[ebevhan]

In D62574#2133160, @danilaml wrote:

What are the remaining roadblocks left before this patch can be merged? I'm interested in having a target-specific way to define the allowed explicit/implicit address space conversions.

This has been on my backburner for various reasons, so I'm not sure what the status is any more. I could try rebasing it and seeing where things are at.
I don't believe that the issues I mentioned last have been dealt with, though.

This was also only an initial concept. I think that even once all the issues with the patch have been ironed out, it would require a round of wider review since it's a fairly hefty API change.

Also, it appears that currently whether implicit casts between pointers of different AS are allowed is determined by the superset relations only, however https://reviews.llvm.org/D73360 introduced another (openCL-specific) variable into the mix: whether the conversion is a top level or not. IMHO, this should also be configured by the target, although I'm not sure whether it needs a separate hook (isBitcastNoop or similar?) or it needs to check the legality of the implicit casts differently.

That patch only seems to apply to C++, I think? The restriction on top-level-only conversion should be correct, at least in C++.
It's generally not safe to alter address spaces below the top level. C is just very permissive about it.

[ebevhan]

It seems that D70605 attempted to ameliorate the issues that I observed (pointer-conversion doing too much), but it didn't manage to solve the problem fully.

[danilaml]

In D62574#2135662, @ebevhan wrote:

It's generally not safe to alter address spaces below the top level. C is just very permissive about it.

Isn't that also true for the top-level casts? Unless when it is. And the target should know when it's safe or not. It's just that for non top-level casts there is added condition that casts are noop (i.e. don't change the bit pattern of the pointer). At least that's how I see it.

I think that you are write about that patch being targeted at C++ (need to do some C tests), but having the same implicit conversion being legal in C and not in C++ would be a bit confusing for the users (especially when the target knows it's completely safe). Anyway, this is not as important as getting this target-configurable AS work done. Without it, I guess the only option for downstream works to customize the behavior is to introduce their own LangAS-level AS entries, which is definitely not ideal.

[ebevhan]

In D62574#2136553, @danilaml wrote:

In D62574#2135662, @ebevhan wrote:

It's generally not safe to alter address spaces below the top level. C is just very permissive about it.

Isn't that also true for the top-level casts? Unless when it is. And the target should know when it's safe or not. It's just that for non top-level casts there is added condition that casts are noop (i.e. don't change the bit pattern of the pointer). At least that's how I see it.

Yes, I see what you mean. I think that the default assumption in Clang at the moment is that it is not safe to do so, hence the current design. Not that there are many targets that use address spaces, so perhaps that assumption is too conservative for ones that do downstream.

[ebevhan]

Rebased.

[Anastasia]

In D62574#2136423, @ebevhan wrote:

It seems that D70605 attempted to ameliorate the issues that I observed (pointer-conversion doing too much), but it didn't manage to solve the problem fully.

Can you explain what's left to be done?

lib/Sema/SemaOverload.cpp

3171

Btw the example that you provide here seems to compile in OpenCL now: https://godbolt.org/z/j9E3s4

[Anastasia]

This was also only an initial concept. I think that even once all the issues with the patch have been ironed out, it would require a round of wider review since it's a fairly hefty API change.

You don't always need a super polished prototype to justify adding new functionality. As soon as your design is clearly explained and agreed by the relevant stakeholders it should be good enough to kick off the work. Once you add some initial implementation it is easier for the community to chip in if this functionality is important enough which I sense might be the case.

[ebevhan]

In D62574#2183294, @Anastasia wrote:

In D62574#2136423, @ebevhan wrote:

It seems that D70605 attempted to ameliorate the issues that I observed (pointer-conversion doing too much), but it didn't manage to solve the problem fully.

Can you explain what's left to be done?

My cache is a bit flushed on this at the moment but I do believe that if the issue described in D83325 is resolved, this patch should work.

lib/Sema/SemaOverload.cpp

3171

That's neat. It does seem like the example in D83325 compiles too, but I suspect it wouldn't work with this patch.

[ebevhan]

Rebased and updated summary.

[ebevhan]

I think this works now. It should probably be given a few more reviewers to have a look. Do you have some suggestions, @Anastasia ?

[Anastasia]

In D62574#2210503, @ebevhan wrote:

I think this works now. It should probably be given a few more reviewers to have a look. Do you have some suggestions, @Anastasia ?

Great! I will look at this again within a week or so. I think it would be good to get some feedback from John @rjmccall too?

[Anastasia]

This change looks good to me in general conceptually as it is completing missing logic in clang that let's targets to customize the address space behavior!

The change also looks good from the implementation side apart from some small details raised in the comments.

The only thing is that it would be good to test the new target setting logic somehow... do you have any ideas in mind? We could think of creating a dummy target for that or adding a dummy setting in existing targets - maybe SPIR could be a candidate. We have done something similar in the past if you look at FakeAddrSpaceMap in LangOpts.

clang/include/clang/AST/ASTContext.h

2371 ↗

(On Diff #284765)

Perhaps not necessarily related to this change but I feel we should provide an explanation of how those functions behave wrt address spaces because terms more qualified or less qualified don't really apply there.

2588 ↗

(On Diff #284765)

I think we should finally start referring to section 5 of embedded C spec as it is not a secret that clang implements exactly this. It is rather a good thing to document the implemented behavior.

2612 ↗

(On Diff #284765)

Here we should say that this is an extension or enhancement of embedded C rules that clang implements.

Technically for OpenCL we could refactor to use this functionality as we don't support such explicit casts on disjoint address spaces. But then this would not be necessarily a target setting.

clang/lib/AST/ASTContext.cpp

10959 ↗

(On Diff #284765)

I guess we should add a similar check here as below?

if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
      From == LangAS::Default || To == LangAS::Default)

10963 ↗

(On Diff #284765)

Btw I assume that explicit cast can't reject what is not rejected by implicit cast?

I am not sure if we need to enforce or document this somehow considering that we provide full configurability now?

clang/lib/Sema/SemaCast.cpp

2423 ↗

(On Diff #284765)

Btw just to point our that overlapping used to be a commutative operation so you could swap arguments and still get the same answer but for isExplicitAddrSpaceConversionLegal is not the same I assume?

clang/lib/Sema/SemaOverload.cpp

3235 ↗

(On Diff #284765)

Sorry if this has been discussed previously, do you refer to the first or the second case and is there any failing test case?

5289 ↗

(On Diff #284765)

Do you have a failing test case, if so feel free to create a bug?

clang/test/CodeGenCXX/address-space-cast.cpp

41 ↗

(On Diff #284765)

I am confused about why this is changed now? adrspacecast can cast a type and an address space too i.e. it is implicitly a bitcast too.

[ebevhan]

In D62574#2242471, @Anastasia wrote:

The only thing is that it would be good to test the new target setting logic somehow... do you have any ideas in mind? We could think of creating a dummy target for that or adding a dummy setting in existing targets - maybe SPIR could be a candidate. We have done something similar in the past if you look at FakeAddrSpaceMap in LangOpts.

Perhaps we could add a configuration to AMDGPU? That has address spaces.

I'm not a big fan of adding an option just for testing.

clang/include/clang/AST/ASTContext.h

2612 ↗

(On Diff #284765)

I'm still a bit on the fence about what Embedded-C really stipulates. I don't think it's against the spec to simply disallow disjoint conversion altogether, but it's only necessary to keep Clang's current implementation working.

clang/lib/AST/ASTContext.cpp

10959 ↗

(On Diff #284765)

Is it not useful for targets to be able to express relations of LangASes and target ASes?

The method below must be guarded because otherwise all casts between LangASes would be legal.

10963 ↗

(On Diff #284765)

It shouldn't do that, no. I don't think there's any way to guarantee this, though.

I could add something to the target methods about it.

clang/lib/Sema/SemaCast.cpp

2423 ↗

(On Diff #284765)

Correct, isExplicitAddrSpaceConversionLegal doesn't have to be commutative.

clang/lib/Sema/SemaOverload.cpp

3235 ↗

(On Diff #284765)

It refers to the first case of "valid to convert to addr space that is a superset in all cases". Technically, it could be permitted even if the addr space is not a superset, if this is an explicit cast. But we don't know that. We only know if it's a c-style cast, because those are always 'explicit'.

I don't have a test case, unfortunately. I just made this observation as I was redoing all of the overlap/superspace checks. It might not even be a problem.

5289 ↗

(On Diff #284765)

Unsure how I'd make one. I suspect this can't be triggered in OpenCL++, because you can't really have LangAS::Default on FromType there, can you? It would always be some AS.

Doing it in another way would require a target that has configurable ASes, which doesn't exist yet. Also, it would require methods qualified with target ASes, and that doesn't work yet either.

clang/test/CodeGenCXX/address-space-cast.cpp

41 ↗

(On Diff #284765)

I don't remember the exact details of how it ends up this way, but it has to do with the way standard conversion sequences are determined. The bitcast and AS-cast are done in two separate steps; the bitcast as part of the pointer conversion in the second SCS step, and the AS-cast as part of the qualification conversion in the third SCS step.

[ebevhan]

clang/lib/AST/ASTContext.cpp

10963 ↗

(On Diff #284765)

Wait, no. This is already guaranteed, because the method here in ASTContext will check for overlap first.

[Anastasia]

In D62574#2249817, @ebevhan wrote:

In D62574#2242471, @Anastasia wrote:

The only thing is that it would be good to test the new target setting logic somehow... do you have any ideas in mind? We could think of creating a dummy target for that or adding a dummy setting in existing targets - maybe SPIR could be a candidate. We have done something similar in the past if you look at FakeAddrSpaceMap in LangOpts.

Perhaps we could add a configuration to AMDGPU? That has address spaces.

I'm not a big fan of adding an option just for testing.

Ok yes if that can be done it would be better. However, it might need extra involvement from AMD side to make sure at least they would agree on maintaining this.

clang/include/clang/AST/ASTContext.h

2612 ↗

(On Diff #284765)

Technically Clang's policy is to implement documented and standardized behavior. So the question is not necessarily about its usefulness, but about adhering to good practices.

I think it is ok to deviate from the standard to make the behavior more helpful in some cases but we should aim at documenting such changes. This will help the developers in the future if they need to fix bugs or build something on top.

clang/lib/AST/ASTContext.cpp

10959 ↗

(On Diff #284765)

I am not sure I follow your comment.

Is it not useful for targets to be able to express relations of LangASes and target ASes?

yes that's why I was thinking we should add a check to make sure we only ask target for target ASes...

However, I am not sure what we should do if there is a mix of target and language AS. I guess this should be governed by the target hooks?

clang/lib/Sema/SemaOverload.cpp

3235 ↗

(On Diff #284765)

Ok, we could update to:
`
This should probably be using isExplicitAddrSpaceConversionLegal -> The first conversion should probably be using isExplicitAddrSpaceConversionLegal too.`

If you already have a failing test case we could create a bug to track this better.

5289 ↗

(On Diff #284765)

Ok, that's right in OpenCL almost every type gets an address space early in parsing.

But if we already know it's a bug and the fix for it we could change this code? Although I think the bug fix should better be done on a separate review.

clang/test/CodeGenCXX/address-space-cast.cpp

41 ↗

(On Diff #284765)

Ah ok, makes sense although the IR will be different to C, but the sematic is the same.

[danilaml]

So is missing an in-tree user (like AMDGPU) the only thing that prevents from merging this?

[rjmccall]

Sorry for never actually reviewing this. I have no objection to taking a refactor that implements the Embedded C address-space overlap rules even if we don't have an in-tree target that uses it. I'll try to find time to review.

[ebevhan]

I've been away from LLVM development for a while, and sadly this patch has languished a bit.

I don't think there were any strong objections to its inclusion, though. If there is still interest in reviewing this, I could try to rebase the patch (or something resembling it) onto the latest main and see if it still works.

[rjmccall]

I completely lost track of this, but if you'd like to rebase it, I can try to make time.

[ebevhan]

Rebased and addressed some comments.

[ebevhan]

Sorry for the delay! I rebased the patch onto latest and it mostly worked, with a few hitches.

clang/lib/Sema/SemaCast.cpp

2617–2619 ↗

(On Diff #444638)

Without this, SemaOpenCL/func.cl fails since foo((void*)foo) is no longer an error. Since the function pointer is Default-qualified, isExplicitAddrSpaceConversionLegal delegates the check to the target, which will always return true.

To be honest, the clause in that method for letting targets allow conversions where LangAS::Default is involved only works so long as every type in OpenCL has the "right" semantic AS. When any of them are Default, things like this happen.

Maybe we should only ask the target about explicit conversions if both ASes are Default or a target AS?

clang/lib/Sema/SemaOverload.cpp

5289 ↗

(On Diff #284765)

I made the fix here just to try, and it caused a difference in address-space-lambda.clcpp.

I'm not sure why that lambda would be valid just because mutable is added... So this seems like the correct behavior to me?

include/clang/AST/ASTContext.h

public:
   /// Get target-dependent integer value for null pointer which is used for
   /// constant folding.
   uint64_t getTargetNullPointerValue(QualType QT) const;
 
   bool addressSpaceMapManglingFor(LangAS AS) const {
     return AddrSpaceMapMangling || isTargetAddressSpace(AS);
   }
 
+  /// Returns true if address space A is a superset of B.
+  bool isAddressSpaceSupersetOf(LangAS A, LangAS B) const {
+    // All address spaces are supersets of themselves.
+    if (A == B)
+      return true;
+
+    // OpenCL v2.0 defines conversion rules (OpenCLC v2.0 s6.5.5) and notion of
+    // overlapping address spaces.
+    // CL1.1 or CL1.2:
+    //   every address space is a superset of itself.
+    // CL2.0 adds:
+    //   __generic is a superset of any address space except for __constant.
+    if (A == LangAS::opencl_generic &&
+        B != LangAS::opencl_constant)
+      return true;
+
+    // Otherwise, ask the target.
+    return Target->isAddressSpaceSupersetOf(A, B);
+  }
+  bool isAddressSpaceSupersetOf(Qualifiers A, Qualifiers B) const {
+    return isAddressSpaceSupersetOf(A.getAddressSpace(), B.getAddressSpace());
+  }
+
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+    // A overlaps with B if either is a superset of the other.
+    return isAddressSpaceSupersetOf(A, B) ||
+           isAddressSpaceSupersetOf(B, A);
+  }
+  bool isAddressSpaceOverlapping(Qualifiers A, Qualifiers B) const {
+    return isAddressSpaceOverlapping(A.getAddressSpace(), B.getAddressSpace());
+  }
+
+  /// Returns true if an explicit cast from address space A to B is legal.
+  bool isExplicitAddrSpaceConversionLegal(LangAS From, LangAS To) const {
+    // If From and To overlap, the cast is legal.
+    if (isAddressSpaceOverlapping(From, To))
+      return true;
+
+    // Or, if either From or To are target address spaces, the target can
+    // decide whether or not to allow the cast regardless of overlap.
+    if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
+        From == LangAS::Default || To == LangAS::Default)
+      return Target->isExplicitAddrSpaceConversionLegal(From, To);
+
+    // Otherwise, the cast is illegal.
+    return false;
+  }
+  bool isExplicitAddrSpaceConversionLegal(Qualifiers From,
+                                          Qualifiers To) const {
+    return isExplicitAddrSpaceConversionLegal(From.getAddressSpace(),
+                                              To.getAddressSpace());
+  }
+
 private:
   // Helper for integer ordering
   unsigned getIntegerRank(const Type *T) const;
 
 public:
   //===--------------------------------------------------------------------===//
   //                    Type Compatibility Predicates
   //===--------------------------------------------------------------------===//

include/clang/AST/Type.h

PointerType(QualType Pointee, QualType CanonicalPtr)
              Pointee->isInstantiationDependentType(),
              Pointee->isVariablyModifiedType(),
              Pointee->containsUnexpandedParameterPack()),
         PointeeType(Pointee) {}
 
 public:
   QualType getPointeeType() const { return PointeeType; }
 
-  /// Returns true if address spaces of pointers overlap.
-  /// OpenCL v2.0 defines conversion rules for pointers to different
-  /// address spaces (OpenCLC v2.0 s6.5.5) and notion of overlapping
-  /// address spaces.
-  /// CL1.1 or CL1.2:
-  ///   address spaces overlap iff they are they same.
-  /// CL2.0 adds:
-  ///   __generic overlaps with any address space except for __constant.
-  bool isAddressSpaceOverlapping(const PointerType &other) const {
-    Qualifiers thisQuals = PointeeType.getQualifiers();
-    Qualifiers otherQuals = other.getPointeeType().getQualifiers();
-    // Address spaces overlap if at least one of them is a superset of another
-    return thisQuals.isAddressSpaceSupersetOf(otherQuals) ||
-           otherQuals.isAddressSpaceSupersetOf(thisQuals);
-  }
-
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 
   void Profile(llvm::FoldingSetNodeID &ID) {
     Profile(ID, getPointeeType());
   }
 
   static void Profile(llvm::FoldingSetNodeID &ID, QualType Pointee) {

include/clang/Basic/TargetInfo.h

public:
   }
 
   /// Specify if mangling based on address space map should be used or
   /// not for language specific address spaces
   bool useAddressSpaceMapMangling() const {
     return UseAddrSpaceMapMangling;
   }
 
+  /// Return true if address space A is a superspace of B.
+  /// By default, all target address spaces are disjoint.
+  virtual bool isAddressSpaceSupersetOf(LangAS A, LangAS B) const {
+    return false;
+  }
+
+  /// Return true if an explicit cast from address space From to To is legal.
+  /// This lets targets override the behavior when neither address space is a
+  /// true superset of the other, but the target still wants explicit casting
+  /// between them.
+  /// By default, explicit casting between all target address spaces is
+  /// permitted.
+  virtual bool isExplicitAddrSpaceConversionLegal(LangAS From, LangAS To) const{
+    return true;
+  }
+
   ///===---- Other target property query methods --------------------------===//
 
   /// Appends the target-specific \#define values for this
   /// target set to the specified buffer.
   virtual void getTargetDefines(const LangOptions &Opts,
                                 MacroBuilder &Builder) const = 0;
 
 

lib/AST/ASTContext.cpp

if (Unqualified) {
       LHSPointee = LHSPointee.getUnqualifiedType();
       RHSPointee = RHSPointee.getUnqualifiedType();
     }
     if (getLangOpts().OpenCL) {
       Qualifiers LHSPteeQual = LHSPointee.getQualifiers();
       Qualifiers RHSPteeQual = RHSPointee.getQualifiers();
       // Blocks can't be an expression in a ternary operator (OpenCL v2.0
       // 6.12.5) thus the following check is asymmetric.
-      if (!LHSPteeQual.isAddressSpaceSupersetOf(RHSPteeQual))
+      if (!isAddressSpaceSupersetOf(LHSPteeQual, RHSPteeQual))
         return {};
       LHSPteeQual.removeAddressSpace();
       RHSPteeQual.removeAddressSpace();
       LHSPointee =
           QualType(LHSPointee.getTypePtr(), LHSPteeQual.getAsOpaqueValue());
       RHSPointee =
           QualType(RHSPointee.getTypePtr(), RHSPteeQual.getAsOpaqueValue());
     }

lib/Sema/SemaCast.cpp

static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,
   if (auto CACK =
           CastsAwayConstness(Self, SrcType, DestType, /*CheckCVR=*/!CStyle,
                              /*CheckObjCLifetime=*/CStyle))
     SuccessResult = getCastAwayConstnessCastKind(CACK, msg);
 
   if (IsAddressSpaceConversion(SrcType, DestType)) {
     Kind = CK_AddressSpaceConversion;
     assert(SrcType->isPointerType() && DestType->isPointerType());
-    if (!CStyle &&
-        !DestType->getPointeeType().getQualifiers().isAddressSpaceSupersetOf(
-            SrcType->getPointeeType().getQualifiers())) {
+    auto SrcQ = SrcType->getPointeeType().getQualifiers();
+    auto DestQ = DestType->getPointeeType().getQualifiers();
+    // Real reinterpret_casts can only do address space conversions which are
+    // 'implicit', such as subspace->superspace. For C-style casts, check if
+    // the cast is explicitly legal as well.
+    if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+               : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {
       SuccessResult = TC_Failed;
     }
   } else if (IsLValueCast) {
     Kind = CK_LValueBitCast;
   } else if (DestType->isObjCObjectPointerType()) {
     Kind = Self.PrepareCastToObjCObjectPointer(SrcExpr);
   } else if (DestType->isBlockPointerType()) {
     if (!SrcType->isBlockPointerType()) {
static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,
   // Void pointers are not specified, but supported by every compiler out there.
   // So we finish by allowing everything that remains - it's got to be two
   // object pointers.
   return SuccessResult;
 }
 
 static TryCastResult TryAddressSpaceCast(Sema &Self, ExprResult &SrcExpr,
                                          QualType DestType, bool CStyle,
-                                         unsigned &msg) {
-  if (!Self.getLangOpts().OpenCL)
-    // FIXME: As compiler doesn't have any information about overlapping addr
-    // spaces at the moment we have to be permissive here.
-    return TC_NotApplicable;
-  // Even though the logic below is general enough and can be applied to
-  // non-OpenCL mode too, we fast-path above because no other languages
-  // define overlapping address spaces currently.
+                                         unsigned &msg, CastKind &Kind) {
   auto SrcType = SrcExpr.get()->getType();
   auto SrcPtrType = SrcType->getAs<PointerType>();
   if (!SrcPtrType)
     return TC_NotApplicable;
   auto DestPtrType = DestType->getAs<PointerType>();
   if (!DestPtrType)
     return TC_NotApplicable;
   auto SrcPointeeType = SrcPtrType->getPointeeType();
   auto DestPointeeType = DestPtrType->getPointeeType();
   if (SrcPointeeType.getAddressSpace() == DestPointeeType.getAddressSpace())
     return TC_NotApplicable;
-  if (!DestPtrType->isAddressSpaceOverlapping(*SrcPtrType)) {
+  if (!Self.Context.isExplicitAddrSpaceConversionLegal(
+        SrcPointeeType.getQualifiers(), DestPointeeType.getQualifiers())) {
     msg = diag::err_bad_cxx_cast_addr_space_mismatch;
     return TC_Failed;
   }
   auto SrcPointeeTypeWithoutAS =
       Self.Context.removeAddrSpaceQualType(SrcPointeeType.getCanonicalType());
   auto DestPointeeTypeWithoutAS =
       Self.Context.removeAddrSpaceQualType(DestPointeeType.getCanonicalType());
-  return Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
-                                  DestPointeeTypeWithoutAS)
-             ? TC_Success
-             : TC_NotApplicable;
+  if (!Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
+                                  DestPointeeTypeWithoutAS))
+    return TC_NotApplicable;
+
+  Kind = CK_AddressSpaceConversion;
+  return TC_Success;
 }
 
 void CastOperation::checkAddressSpaceCast(QualType SrcType, QualType DestType) {
   // In OpenCL only conversions between pointers to objects in overlapping
   // addr spaces are allowed. v2.0 s6.5.5 - Generic addr space overlaps
   // with any named one, except for constant.
 
   // Converting the top level pointee addrspace is permitted for compatible
   // addrspaces (such as 'generic int *' to 'local int *' or vice versa), but
   // if any of the nested pointee addrspaces differ, we emit a warning
   // regardless of addrspace compatibility. This makes
   //   local int ** p;
   //   return (generic int **) p;
   // warn even though local -> generic is permitted.
-  if (Self.getLangOpts().OpenCL) {
-    const Type *DestPtr, *SrcPtr;
-    bool Nested = false;
-    unsigned DiagID = diag::err_typecheck_incompatible_address_space;
-    DestPtr = Self.getASTContext().getCanonicalType(DestType.getTypePtr()),
-    SrcPtr  = Self.getASTContext().getCanonicalType(SrcType.getTypePtr());
 
-    while (isa<PointerType>(DestPtr) && isa<PointerType>(SrcPtr)) {
-      const PointerType *DestPPtr = cast<PointerType>(DestPtr);
-      const PointerType *SrcPPtr = cast<PointerType>(SrcPtr);
-      QualType DestPPointee = DestPPtr->getPointeeType();
-      QualType SrcPPointee = SrcPPtr->getPointeeType();
-      if (Nested ? DestPPointee.getAddressSpace() !=
-                   SrcPPointee.getAddressSpace()
-                 : !DestPPtr->isAddressSpaceOverlapping(*SrcPPtr)) {
-        Self.Diag(OpRange.getBegin(), DiagID)
-            << SrcType << DestType << Sema::AA_Casting
-            << SrcExpr.get()->getSourceRange();
-        if (!Nested)
-          SrcExpr = ExprError();
-        return;
-      }
+               : !Self.Context.isExplicitAddrSpaceConversionLegal(
+                   SrcPPointee.getAddressSpace(),
+                   DestPPointee.getAddressSpace())) {
 
-      DestPtr = DestPPtr->getPointeeType().getTypePtr();
-      SrcPtr = SrcPPtr->getPointeeType().getTypePtr();
-      Nested = true;
-      DiagID = diag::ext_nested_pointer_qualifier_mismatch;
-    }
-  }
-}
 
 void CastOperation::CheckCXXCStyleCast(bool FunctionalStyle,
                                        bool ListInitialization) {
   assert(Self.getLangOpts().CPlusPlus);
 
   // Handle placeholders.
   if (isPlaceholder()) {
     // C-style casts can resolve __unknown_any types.
void CastOperation::CheckCXXCStyleCast(bool FunctionalStyle,
   if (SrcExpr.isInvalid())
     return;
   if (isValidCast(tcr))
     Kind = CK_NoOp;
 
   Sema::CheckedConversionKind CCK =
       FunctionalStyle ? Sema::CCK_FunctionalCast : Sema::CCK_CStyleCast;
   if (tcr == TC_NotApplicable) {
-    tcr = TryAddressSpaceCast(Self, SrcExpr, DestType, /*CStyle*/ true, msg);
+    tcr = TryAddressSpaceCast(Self, SrcExpr, DestType, /*CStyle*/ true, msg,
+                              Kind);
     if (SrcExpr.isInvalid())
       return;
     if (tcr == TC_NotApplicable) {
       // ... or if that is not possible, a static_cast, ignoring const, ...
       tcr = TryStaticCast(Self, SrcExpr, DestType, CCK, OpRange, msg, Kind,
                           BasePath, ListInitialization);
       if (SrcExpr.isInvalid())
         return;

lib/Sema/SemaExpr.cpp

static QualType checkConditionalPointerCompatibility(Sema &S, ExprResult &LHS,
   Qualifiers rhQual = rhptee.getQualifiers();
 
   LangAS ResultAddrSpace = LangAS::Default;
   LangAS LAddrSpace = lhQual.getAddressSpace();
   LangAS RAddrSpace = rhQual.getAddressSpace();
 
   // OpenCL v1.1 s6.5 - Conversion between pointers to distinct address
   // spaces is disallowed.
-  if (lhQual.isAddressSpaceSupersetOf(rhQual))
+  if (S.Context.isAddressSpaceSupersetOf(lhQual, rhQual))
     ResultAddrSpace = LAddrSpace;
-  else if (rhQual.isAddressSpaceSupersetOf(lhQual))
+  else if (S.Context.isAddressSpaceSupersetOf(rhQual, lhQual))
     ResultAddrSpace = RAddrSpace;
   else {
     S.Diag(Loc, diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
         << LHSTy << RHSTy << 2 << LHS.get()->getSourceRange()
         << RHS.get()->getSourceRange();
     return QualType();
   }
 
if (lhq.getObjCLifetime() != rhq.getObjCLifetime() &&
       lhq.compatiblyIncludesObjCLifetime(rhq)) {
     // Ignore lifetime for further calculation.
     lhq.removeObjCLifetime();
     rhq.removeObjCLifetime();
   }
 
   if (!lhq.compatiblyIncludes(rhq)) {
     // Treat address-space mismatches as fatal.
-    if (!lhq.isAddressSpaceSupersetOf(rhq))
+    if (!S.Context.isAddressSpaceSupersetOf(lhq, rhq))
       return Sema::IncompatiblePointerDiscardsQualifiers;
 
     // It's okay to add or remove GC or lifetime qualifiers when converting to
     // and from void*.
     else if (lhq.withoutObjCGCAttr().withoutObjCLifetime()
                         .compatiblyIncludes(
                                 rhq.withoutObjCGCAttr().withoutObjCLifetime())
              && (lhptee->isVoidType() || rhptee->isVoidType()))
static bool checkArithmeticBinOpPointerOperands(Sema &S, SourceLocation Loc,
   bool isRHSPointer = RHSExpr->getType()->isAnyPointerType();
   if (!isLHSPointer && !isRHSPointer) return true;
 
   QualType LHSPointeeTy, RHSPointeeTy;
   if (isLHSPointer) LHSPointeeTy = LHSExpr->getType()->getPointeeType();
   if (isRHSPointer) RHSPointeeTy = RHSExpr->getType()->getPointeeType();
 
   // if both are pointers check if operation is valid wrt address spaces
-  if (S.getLangOpts().OpenCL && isLHSPointer && isRHSPointer) {
+  if (isLHSPointer && isRHSPointer) {
     const PointerType *lhsPtr = LHSExpr->getType()->getAs<PointerType>();
     const PointerType *rhsPtr = RHSExpr->getType()->getAs<PointerType>();
-    if (!lhsPtr->isAddressSpaceOverlapping(*rhsPtr)) {
+    if (!S.Context.isAddressSpaceOverlapping(
+          lhsPtr->getPointeeType().getAddressSpace(),
+          rhsPtr->getPointeeType().getAddressSpace())) {
       S.Diag(Loc,
              diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
           << LHSExpr->getType() << RHSExpr->getType() << 1 /*arithmetic op*/
           << LHSExpr->getSourceRange() << RHSExpr->getSourceRange();
       return false;
     }
   }
 
if (Context.typesAreCompatible(LCanPointeeTy.getUnqualifiedType(),
           && !LHSIsNull && !RHSIsNull)
         diagnoseFunctionPointerToVoidComparison(*this, Loc, LHS, RHS,
                                                 /*isError*/false);
     } else {
       // Invalid
       diagnoseDistinctPointerComparison(*this, Loc, LHS, RHS, /*isError*/false);
     }
     if (LCanPointeeTy != RCanPointeeTy) {
-      // Treat NULL constant as a special case in OpenCL.
-      if (getLangOpts().OpenCL && !LHSIsNull && !RHSIsNull) {
+      if (!LHSIsNull && !RHSIsNull) {
         const PointerType *LHSPtr = LHSType->getAs<PointerType>();
-        if (!LHSPtr->isAddressSpaceOverlapping(*RHSType->getAs<PointerType>())) {
+        if (!Context.isAddressSpaceOverlapping(
+              LHSPtr->getPointeeType().getAddressSpace(),
+              RHSType->getPointeeType().getAddressSpace())) {
           Diag(Loc,
                diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
               << LHSType << RHSType << 0 /* comparison */
               << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
         }
       }
       LangAS AddrSpaceL = LCanPointeeTy.getAddressSpace();
       LangAS AddrSpaceR = RCanPointeeTy.getAddressSpace();

lib/Sema/SemaInit.cpp

static void TryReferenceInitializationCore(Sema &S,
 
   //        [...] If T1 is reference-related to T2, cv1 must be the
   //        same cv-qualification as, or greater cv-qualification
   //        than, cv2; otherwise, the program is ill-formed.
   unsigned T1CVRQuals = T1Quals.getCVRQualifiers();
   unsigned T2CVRQuals = T2Quals.getCVRQualifiers();
   if ((RefRelationship == Sema::Ref_Related &&
        (T1CVRQuals | T2CVRQuals) != T1CVRQuals) ||
-      !T1Quals.isAddressSpaceSupersetOf(T2Quals)) {
+      !S.Context.isAddressSpaceSupersetOf(T1Quals, T2Quals)) {
     Sequence.SetFailed(InitializationSequence::FK_ReferenceInitDropsQualifiers);
     return;
   }
 
   //   [...] If T1 is reference-related to T2 and the reference is an rvalue
   //   reference, the initializer expression shall not be an lvalue.
   if (RefRelationship >= Sema::Ref_Related && !isLValueRef &&
       InitCategory.isLValue()) {

lib/Sema/SemaOverload.cpp

if (!CStyle && FromQuals.getCVRQualifiers() != ToQuals.getCVRQualifiers()
       return false;
 
     // Keep track of whether all prior cv-qualifiers in the "to" type
     // include const.
     PreviousToQualsIncludeConst
       = PreviousToQualsIncludeConst && ToQuals.hasConst();
   }
 
-  // Allows address space promotion by language rules implemented in
-  // Type::Qualifiers::isAddressSpaceSupersetOf.
+  // FIXME: This should *really* be testing implicit conversions with
+  // 'isAddressSpaceSupersetOf' and explicit conversions with
+  // 'isExplicitAddrSpaceConversionLegal', but IsQualificationConversion isn't
+  // aware of whether the conversion is implicit or explicit. Add that?
+  //
+  // Also, I don't really understand why we do this. How does preventing
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();
   Qualifiers ToQuals = ToType.getQualifiers();
-  if (!ToQuals.isAddressSpaceSupersetOf(FromQuals) &&
-      !FromQuals.isAddressSpaceSupersetOf(ToQuals)) {
+  if (!Context.isAddressSpaceOverlapping(ToQuals, FromQuals)) {
     return false;
   }
 
   // We are left with FromType and ToType being the pointee types
   // after unwrapping the original FromType and ToType the same number
   // of types. If we unwrapped any pointers, and if FromType and
   // ToType have the same unqualified type (since we checked
   // qualifiers above), then this is a qualification conversion.
TryObjectArgumentInitialization(Sema &S, SourceLocation Loc, QualType FromType,
   if (ImplicitParamType.getCVRQualifiers()
                                     != FromTypeCanon.getLocalCVRQualifiers() &&
       !ImplicitParamType.isAtLeastAsQualifiedAs(FromTypeCanon)) {
     ICS.setBad(BadConversionSequence::bad_qualifiers,
                FromType, ImplicitParamType);
     return ICS;
   }
 
+  // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is
+  // not qualified with an address space, but if there's no implicit conversion
+  // from Default to ImplicitParamType's AS, that's an error.
   if (FromTypeCanon.getQualifiers().hasAddressSpace()) {
     Qualifiers QualsImplicitParamType = ImplicitParamType.getQualifiers();
     Qualifiers QualsFromType = FromTypeCanon.getQualifiers();
-    if (!QualsImplicitParamType.isAddressSpaceSupersetOf(QualsFromType)) {
+    if (!S.Context.isAddressSpaceSupersetOf(QualsImplicitParamType,
+                                            QualsFromType)) {
       ICS.setBad(BadConversionSequence::bad_qualifiers,
                  FromType, ImplicitParamType);
       return ICS;
     }
   }
 
   // Check that we have either the same type or a derived type. It
   // affects the conversion rank.

test/Sema/address_space_print_macro.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify
 
 #define AS1 __attribute__((address_space(1)))
 #define AS2 __attribute__((address_space(2), annotate("foo")))
 #define AS_ND __attribute__((address_space(2), noderef))
 
 #define AS(i) address_space(i)
 #define AS3 __attribute__((AS(3)))
 #define AS5 __attribute__((address_space(5))) char
 
 void normal_case() {
   int *p = 0;
   __attribute__((address_space(1))) int *q = p; // expected-error{{initializing '__attribute__((address_space(1))) int *' with an expression of type 'int *' changes address space of pointer}}
 }
 
 char *cmp(AS1 char *x, AS2 char *y) {
-  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}}
+  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}} \
+                        // expected-error{{comparison between  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}}
 }
 
 __attribute__((address_space(1))) char test_array[10];
 void test3(void) {
   extern void test3_helper(char *p); // expected-note{{passing argument to parameter 'p' here}}
   test3_helper(test_array);          // expected-error{{passing '__attribute__((address_space(1))) char *' to parameter of type 'char *' changes address space of pointer}}
 }
 

test/Sema/address_spaces.c

void access_as_field()
     (void) bar.as_field;
 }
 
 typedef int PR4997 __attribute__((address_space(Foobar))); // expected-error {{use of undeclared identifier 'Foobar'}}
 __attribute__((address_space("12"))) int *i; // expected-error {{'address_space' attribute requires an integer constant}}
 
 // Clang extension doesn't forbid operations on pointers to different address spaces.
 char* cmp(_AS1 char *x,  _AS2 char *y) {
-  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
+  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}} \
+                        // expected-error{{comparison between  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
 }
 
 struct SomeStruct {
   int a;
   long b;
   int c;
 };
 
 // Compound literals in function scope are lvalues with automatic storage duration,
 // so they cannot realistically be qualified with an address space.
 void as_compound_literal() {
   (_AS1 struct SomeStruct){1, 2, 3}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
   (_AS1 char[]){"test"}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
   (_AS1 char[]){'a', 'b', 'c'}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
 }