Page MenuHomePhabricator

Initial draft of target-configurable address spaces.
Needs ReviewPublic

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

Details

Summary

This is an initial draft of the support for target-configurable
address spaces.

Original RFC: http://lists.llvm.org/pipermail/cfe-dev/2019-March/061541.html

After thinking about Anastasia's input on the RFC regarding
the superspace/subspace model, I decided to go a different
route with the design of the interface. Instead of having
accessors for implicit casts and explicit casts, I kept the
subspace accessor and added an accessor for overriding
the behavior of explicit casts only.

This lets us keep the overlap model and support the default
Embedded-C functionality while still letting targets
configure their behavior reasonably.

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

Diff Detail

Event Timeline

ebevhan created this revision.May 29 2019, 2:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 2:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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!

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.

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

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
2619

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

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

2319

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 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
2619

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

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

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
2619

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

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

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

Should this also be allowed if pointers are explicitly convertible?

lib/Sema/SemaOverload.cpp
3171

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
2619

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

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

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 added inline comments.Jul 30 2019, 10:22 AM
include/clang/AST/ASTContext.h
2619

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

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.