ebevhan added a comment.
In D62574#2242471 <https://reviews.llvm.org/D62574#2242471>, @Anastasia wrote:
> The only thing is that it would be good to test the new target setting logic
> somehow... do you have any ideas in mind? We could think of creating a dummy
> target for that or adding a dummy setting in existing targets - maybe SPIR
> could be a candidate. We have done something similar in the past if you look
> at `FakeAddrSpaceMap` in `LangOpts`.
Perhaps we could add a configuration to AMDGPU? That has address spaces.
I'm not a big fan of adding an option just for testing.
================
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
----------------
Anastasia wrote:
> 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.
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.
================
Comment at: clang/lib/AST/ASTContext.cpp:10959
+ // Otherwise, ask the target.
+ return Target->isAddressSpaceSupersetOf(A, B);
+}
----------------
Anastasia wrote:
> I guess we should add a similar check here as below?
>
>
> ```
> if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
> From == LangAS::Default || To == LangAS::Default)
> ```
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.
================
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.
----------------
Anastasia wrote:
> 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?
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.
================
Comment at: clang/lib/Sema/SemaCast.cpp:2423
auto DestPointeeType = DestPtrType->getPointeeType();
- if (!DestPointeeType.isAddressSpaceOverlapping(SrcPointeeType)) {
+ if (!Self.Context.isExplicitAddrSpaceConversionLegal(
+ SrcPointeeType.getQualifiers(), DestPointeeType.getQualifiers())) {
----------------
Anastasia wrote:
> 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?
Correct, isExplicitAddrSpaceConversionLegal doesn't have to be commutative.
================
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.
----------------
Anastasia wrote:
> Sorry if this has been discussed previously, do you refer to the first or the
> second case and is there any failing test case?
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.
================
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
----------------
Anastasia wrote:
> Do you have a failing test case, if so feel free to create a bug?
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.
================
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]]
----------------
Anastasia wrote:
> 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.
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.
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
