Anastasia added a comment.
> The feature is also C++ for OpenCL specific to let the fast path remain when
> not utilizing the
> address spaces, as this new implementation will be slower than the bitfields
> that C++ currently
> uses, but I hope this can also be optimized in the future if it turns out to
> be very slow.
In general we try to align OpenCL's address spaces with C/C++ general direction
but we haven't completed support of address space methods qualifiers in C++
yet. Therefore supporting dtors with address spaces won't be possible in
generic C++. However, it would be good to check that we don't entirely misalign
with the C++ requirements.
I would like to see if @rjmccall has any suggestions at this point.
================
Comment at: clang/include/clang/AST/DeclCXX.h:983
+ bool needsImplicitDestructor(LangAS AS = LangAS::Default) const {
+ if (getASTContext().getLangOpts().OpenCL) {
+ return getDestructor(AS) == nullptr;
----------------
According to the current OpenCL strategy, we only declare implicit members in
default address space (i.e. `__generic`). It might be that we can declare them
for all concrete address spaces but we are not there yet to start adding this
feature. So it would make sense, for now, to simplify this logic and only add
implicit dtors in the generic address space.
This implies that objects in concrete address spaces would still be able to use
such implicit dtors but the compiler would add an implicit conversion to
`__generic` for the implicit object parameter.
================
Comment at: clang/include/clang/Sema/Sema.h:4048
+ bool VolatileArg, bool RValueThis, bool ConstThis,
+ bool VolatileThis, LangAS AS = LangAS::Default);
----------------
it feels to me that this should be named `ASThis` following the convention?
================
Comment at: clang/lib/AST/DeclCXX.cpp:1901
+
+ for (DeclContext::lookup_result::iterator I = R.begin(), E = R.end(); I != E;
+ ++I) {
----------------
For ctors and other special members the logic for the selection of the overload
candidates seems to be implemented at the end of `Sema::LookupSpecialMember`:
https://clang.llvm.org/doxygen/SemaLookup_8cpp_source.html#l03114
There it creates an implicit parameter `this` and performs regular overload
resolution.
Currently dtors don't hit that point because the implementation fast paths them
in that function.
Do you think it makes sense to follow the same logic for dtors (by preventing
the early exit there), even though perhaps we would then only do it for OpenCL
to avoid performance regressions for pure C++ implementation? However, I am not
entirely convinced when and how that code is called for the special members
since it doesn't have address space handling anywhere but clang's behavior
seems correct for ctors and assignments at least. Although it is possible that
it is only used for some corner cases we haven't encountered yet and hence we
have a general bug for special members with address spaces in some situations.
Alternatively, we could just add similar logic here if it makes implementation
more efficient. After all the special member implementation doesn't seem to
reuse fully common member function overload resolution code so perhaps it is
not unreasonable to add a special case for dtors. I am not saying though we
need to do it in this path... but it would be good to understand how the
evolution of your approach would look like towards completion.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3082
ID.AddInteger(VolatileThis);
+ ID.AddInteger((unsigned)AS);
----------------
Btw ctors and assignments don't seem to need this but they seem to work fine
though...
For example here the right assignment overload with `__local` address space is
selected
https://godbolt.org/z/aYKj4W6rc
or ctor overload with `__global` is selected here correctly:
https://godbolt.org/z/1frheezE5
So it seems like there is some other logic to handle address spaces for special
members elsewhere? Although it is very strange and rather confusing.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3099
+ runWithSufficientStackSpace(RD->getLocation(),
+ [&] { DeclareImplicitDestructor(RD, AS); });
}
----------------
Currently, all other special members added implicitly use default address space
(i.e. `__generic` for OpenCL) for `this` via
`Sema::setupImplicitSpecialMemberType` helper so I think we should mirror it
for `DeclareImplicitDestructor` too... then perhaps we can avoid changes in
`Sema::setupImplicitSpecialMemberType`?
FYI in the past we have discussed that we might want to change the design and
put special members in concrete address spaces but we haven't gone that far yet
and it would make sense to have all special members working coherently... so it
would be better for dtors to just mirror logic from the other special members
for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109609/new/
https://reviews.llvm.org/D109609
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
