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.Wed, Jun 26, 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.Wed, Jun 26, 8:46 AM

Rebased and addressed some comments.

Anastasia added inline comments.Tue, Jul 2, 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?