Anastasia added a comment.
Herald added a subscriber: bjope.
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`.
================
Comment at: clang/include/clang/AST/ASTContext.h:2371
+ /// can be safely used as an object qualified with A.
+ bool compatiblyIncludes(Qualifiers A, Qualifiers B) const {
+ return isAddressSpaceSupersetOf(A, B) &&
----------------
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.
================
Comment at: clang/include/clang/AST/ASTContext.h:2588
+ /// Returns true if address space A is a superset of B.
+ /// The subspace/superspace relation governs the validity of implicit
----------------
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.
================
Comment at: clang/include/clang/AST/ASTContext.h:2612
+
+ /// Returns true if an explicit cast from address space A to B is legal.
+ /// Explicit conversion between address spaces is permitted if the address
----------------
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.
================
Comment at: clang/lib/AST/ASTContext.cpp:10959
+ // Otherwise, ask the target.
+ return Target->isAddressSpaceSupersetOf(A, B);
+}
----------------
I guess we should add a similar check here as below?
```
if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
From == LangAS::Default || To == LangAS::Default)
```
================
Comment at: clang/lib/AST/ASTContext.cpp:10963
+bool
+ASTContext::isExplicitAddrSpaceConversionLegal(LangAS From, LangAS To) const {
+ // If From and To overlap, the cast is legal.
----------------
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?
================
Comment at: clang/lib/Sema/SemaCast.cpp:2423
auto DestPointeeType = DestPtrType->getPointeeType();
- if (!DestPointeeType.isAddressSpaceOverlapping(SrcPointeeType)) {
+ if (!Self.Context.isExplicitAddrSpaceConversionLegal(
+ SrcPointeeType.getQualifiers(), DestPointeeType.getQualifiers())) {
----------------
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?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:3235
// - in non-top levels it is not a valid conversion.
+ // FIXME: This should probably be using isExplicitAddrSpaceConversionLegal,
+ // but we don't know if this is an implicit or explicit conversion.
----------------
Sorry if this has been discussed previously, do you refer to the first or the
second case and is there any failing test case?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:5289
+ // 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
----------------
Do you have a failing test case, if so feel free to create a bug?
================
Comment at: clang/test/CodeGenCXX/address-space-cast.cpp:41
+ // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+ // CHECK-NEXT: %[[cast2:.*]] = addrspacecast i8* %[[cast]] to i8
addrspace(5)*
+ // CHECK-NEXT: store i8 addrspace(5)* %[[cast2]]
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62574/new/
https://reviews.llvm.org/D62574
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
