Page MenuHomePhabricator

Add support for target-configurable address spaces.
Needs ReviewPublic

Authored by ebevhan on May 29 2019, 2:12 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 ↗(On Diff #201842)

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 ↗(On Diff #201842)

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.

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 ↗(On Diff #201842)

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 ↗(On Diff #201842)

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. :)

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 updated this revision to Diff 203559.Jun 7 2019, 8:11 AM

Replaced compatiblyIncludes and its dependents with ASTContext accessors instead.

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
2598 ↗(On Diff #203559)

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
2224 ↗(On Diff #203559)

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

2319 ↗(On Diff #203559)

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

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

2334 ↗(On Diff #203559)

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

ebevhan marked 3 inline comments as done.Jun 26 2019, 7:15 AM

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
2598 ↗(On Diff #203559)

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
2224 ↗(On Diff #203559)

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.

2319 ↗(On Diff #203559)

Oh, indeed.

ebevhan updated this revision to Diff 206691.Jun 26 2019, 8:46 AM

Rebased and addressed some comments.

Anastasia added inline comments.Jul 2 2019, 3:32 AM
include/clang/AST/ASTContext.h
2598 ↗(On Diff #203559)

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
2224 ↗(On Diff #203559)

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?

2325 ↗(On Diff #206691)

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
10616 ↗(On Diff #206691)

Should this also be allowed if pointers are explicitly convertible?

lib/Sema/SemaOverload.cpp
3171 ↗(On Diff #201842)

Did you have time to look into this?

ebevhan marked 4 inline comments as done.Jul 29 2019, 7:32 AM

Sorry for the very late response to this!

include/clang/AST/ASTContext.h
2598 ↗(On Diff #203559)

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
2224 ↗(On Diff #203559)

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
10616 ↗(On Diff #206691)

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 ↗(On Diff #201842)

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 added inline comments.Jul 30 2019, 10:22 AM
include/clang/AST/ASTContext.h
2598 ↗(On Diff #203559)

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
2224 ↗(On Diff #203559)

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 ↗(On Diff #201842)

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 marked 2 inline comments as done.Nov 22 2019, 7:52 AM

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

include/clang/AST/ASTContext.h
2598 ↗(On Diff #203559)

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

lib/Sema/SemaCast.cpp
2224 ↗(On Diff #203559)

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 ↗(On Diff #201842)

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.

ilya added a subscriber: ilya.Nov 26 2019, 8:58 AM
danilaml removed a subscriber: 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.

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.

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.

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.

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.

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 ↗(On Diff #201842)

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

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.

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 ↗(On Diff #201842)

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

ebevhan updated this revision to Diff 284765.Aug 11 2020, 8:51 AM
ebevhan retitled this revision from Initial draft of target-configurable address spaces. to Add support for target-configurable address spaces..
ebevhan edited the summary of this revision. (Show Details)

Rebased and updated summary.

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

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?

bader added a subscriber: bader.Aug 14 2020, 2:30 PM

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

Correct, isExplicitAddrSpaceConversionLegal doesn't have to be commutative.

clang/lib/Sema/SemaOverload.cpp
3235

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

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

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 added inline comments.Sep 2 2020, 5:18 AM
clang/lib/AST/ASTContext.cpp
10963

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

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

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

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

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

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

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