ebevhan marked 3 inline comments as done.
ebevhan added a comment.
In D62574#1552220 <https://reviews.llvm.org/D62574#1552220>, @Anastasia wrote:
> 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.
================
Comment at: include/clang/AST/ASTContext.h:2598
+ /// Returns true if address space A overlaps with B.
+ bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+ // A overlaps with B if either is a superset of the other.
----------------
Anastasia wrote:
> 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?
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.
================
Comment at: lib/Sema/SemaCast.cpp:2224
+ // the cast is explicitly legal as well.
+ if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+ : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {
----------------
Anastasia wrote:
> It seems like you are changing the functionality here. Don't we need any test
> for this?
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.
================
Comment at: lib/Sema/SemaCast.cpp:2319
+
+ Kind = CK_AddressSpaceConversion;
+ return TC_Success;
----------------
Anastasia wrote:
> Btw I think this is set in:
> https://reviews.llvm.org/D62299
>
> Although I don't have any objections to changing to this.
Oh, indeed.
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
