Anastasia marked an inline comment as done.
Anastasia added inline comments.
================
Comment at: lib/Sema/TreeTransform.h:5363
+ if (ResultType.getAddressSpace() != LangAS::Default &&
+ (ResultType.getAddressSpace() != LangAS::opencl_private)) {
SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),
----------------
rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > Anastasia wrote:
> > > > > rjmccall wrote:
> > > > > > Anastasia wrote:
> > > > > > > I am trying to be a bit more helpful here although I am not sure
> > > > > > > if we should instead require explicit template parameter and fail
> > > > > > > the template deduction instead.
> > > > > > >
> > > > > > > Basically, do we want the following code to always require
> > > > > > > specifying template argument explicitly:
> > > > > > >
> > > > > > >
> > > > > > > ```
> > > > > > > template <class T>
> > > > > > > T xxx(T *in) {
> > > > > > > T *i = in;
> > > > > > > return *i;
> > > > > > > }
> > > > > > >
> > > > > > > __kernel void test() {
> > > > > > > int foo[10];
> > > > > > > xxx<int>(&foo[0]); // if we deduce type from foo, it ends up
> > > > > > > being qualified by __private that we currently diagnose. However
> > > > > > > private is default (implicit) address space for return type so
> > > > > > > technically there is no danger in just allowing xxx(&foo[0])
> > > > > > > }
> > > > > > > ```
> > > > > > Implicitly ignoring all address-space qualifiers on the return type
> > > > > > seems like the right thing to do; I don't think it needs to be
> > > > > > limited to `__private`. That's probably also the right thing to do
> > > > > > for locals, but I'm less certain about it.
> > > > > Just to clarify by "Implicitly ignoring" you mean ignoring if the
> > > > > templ parameters were deduced?
> > > > >
> > > > > Although I am a bit concerned about allowing other than `__private`
> > > > > address spaces in return types as we reject them in return types of
> > > > > functions generally. Would it not be somehow inconsistent?
> > > > Ok, I have removed the diagnostic completely. At least we don't seem to
> > > > generate any incorrect IR.
> > > They should be diagnosed somehow when written explicitly on a return
> > > type, but if you just do that on the parser path you don't have to worry
> > > about it during template instantiation. They should probably otherwise
> > > be ignored no matter where they came from — if someone typedefs
> > > `private_int_t` to `__private int`, you should just treat that as `int`
> > > in a return type. Stripping the qualifier from the type is probably the
> > > right thing to do so that it doesn't further impact semantic analysis.
> > >
> > > I definitely don't think you want a model where the qualifier actually
> > > means that the return is somehow done via an object in that address space.
> > > They should be diagnosed somehow when written explicitly on a return
> > > type, but if you just do that on the parser path you don't have to worry
> > > about it during template instantiation.
> >
> > Ok, this seems to be working currently. The following won't compile:
> >
> > ```
> > template <typename T>
> > struct S {
> > __global T f1(); // error: return value cannot be qualified with
> > address space
> > };
> >
> > ```
> >
> > > They should probably otherwise be ignored no matter where they came from
> > > — if someone typedefs private_int_t to __private int, you should just
> > > treat that as int in a return type.
> >
> > We produce diagnostic for this case currently. I can update this in a
> > separate patch if you think it's more helpful behavior. Although I feel a
> > bit worried about it. However it would align with what we are doing with
> > templates here... so perhaps it's logical change...
> >
> > > Stripping the qualifier from the type is probably the right thing to do
> > > so that it doesn't further impact semantic analysis.
> >
> > I guess you mean stripping quals for the case of typedef or others where we
> > will accept the code and ignore quals on the return type? I have tried to
> > do this just for the template case but there are some assertions firing
> > because we are expecting the original return type to be the same before and
> > after template instantiation. So it feels like I would have to do it for
> > everything together. Maybe it should rather go into separate review?
> > We produce diagnostic for this case currently. I can update this in a
> > separate patch if you think it's more helpful behavior. Although I feel a
> > bit worried about it. However it would align with what we are doing with
> > templates here... so perhaps it's logical change...
>
> Well, it's debatable. C doesn't say that qualifiers are erased in function
> return types in general. On the other hand, qualifiers are semantically
> meaningless there, and C has few rules that rely on exact type identity. On
> reflection, we should probably keep diagnosing that at parse-time, even for
> typedefs, and just ignore it in templates.
>
> Can you point out which assertion is firing?
> On reflection, we should probably keep diagnosing that at parse-time, even
> for typedefs, and just ignore it in templates.
In that case we don't need to fix anything in parsing.
> Can you point out which assertion is firing?
I have tried this:
```
diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h
index 878795bb70..db4ae6e56e 100644
--- a/lib/Sema/TreeTransform.h
+++ b/lib/Sema/TreeTransform.h
@@ -5365,6 +5365,11 @@ QualType
TreeTransform<Derived>::TransformFunctionProtoType(
if (ResultType.isNull())
return QualType();
+ if (ResultType.hasQualifiers()) {
+ ResultType = ResultType.getUnqualifiedType();
+ TLB.TypeWasModifiedSafely(ResultType);
+ }
+
if (getDerived().TransformFunctionTypeParams(
TL.getBeginLoc(), TL.getParams(),
TL.getTypePtr()->param_type_begin(),
```
and I am getting some errors in tests mainly because of the differences in the
return type. I.e.
```
error: 'error' diagnostics expected but not seen:
File llvm/tools/clang/test/CXX/dcl.decl/dcl.init/dcl.init.ref/p5-var.cpp Line
126: binding reference of type 'const Base' to value of type 'const volatile
Base' drops 'volatile' qualifier
File llvm/tools/clang/test/CXX/dcl.decl/dcl.init/dcl.init.ref/p5-var.cpp Line
127: binding reference of type 'const Base' to value of type 'const volatile
Derived' drops 'volatile' qualifier
```
Practically we no longer get diagnostics for cases like:
```
48 template<typename T> T create();
126 const Base &br5 = create<const volatile Base>();
127 const Base &br6 = create<const volatile Derived>();
```
Also the qualifiers are not printed in the type of return type.
I guess this is expected. I am however struggling with this assert in a couple
tests:
llvm/tools/clang/lib/Sema/TypeLocBuilder.cpp:160: clang::TypeLoc
clang::TypeLocBuilder::pushImpl(clang::QualType, size_t, unsigned int):
Assertion `Capacity - Index == TypeLoc::getFullDataSizeForType(T) && "incorrect
data size provided to CreateTypeSourceInfo!"' failed.
I am not sure if I am modifying `TypeLocBuilder` in a wrong way or perhaps
something isn't working well in the `Index` calculation. If this is the
direction we would like to go I can spend more time debugging this and upload
the review. Not sure if that might be too disruptive change for the release but
I would at least like to see the current patch in Clang9 because it fixes a
couple of useful cases. :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62584/new/
https://reviews.llvm.org/D62584
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
