[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+
----------------
Anastasia wrote:
> rjmccall wrote:
> > Since you're moving this code anyway, can this be split into its own function?  I'm not sure if it's actually important that some of these failures return immediately and some of them fall through to later checks.
> >  I'm not sure if it's actually important that some of these failures return immediately and some of them fall through to later checks.
> 
> Yes, it looks a bit random. Do we have any guideline whether we should return or keep diagnosing as long as possible?
> 
> 
If the user is likely to have made multiple independent errors, it's good to diagnose as many of them as possible.  But if it's just as likely that the user messed up in a single way that should've meant we didn't take this code path, then it's better to bail out early.

In this case, most of these diagnostics are looking for different special OpenCL types, and those are probably all independent  of each other.

...it does occur to me to wonder if more of these checks should be looking through array types the way that the check for `half` does.  Presumably you shouldn't be able to declare global arrays of images or pipes if you can't declare non-array variables of them.


================
Comment at: clang/lib/Sema/SemaType.cpp:7460
+      // the initializing expression type during the type deduction.
+      (T->isUndeducedAutoType() && IsPointee) ||
       // OpenCL spec v2.0 s6.9.b:
----------------
Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Okay, I understand why you're doing this now, and it makes sense.  I would like to propose changing the entire way `deduceOpenCLImplicitAddrSpace` works.  Why don't we do it more like how ObjC ARC infers its implicit ownership qualifiers:
> > > > 
> > > > - In SemaType, we add the default address space to non-qualified, non-dependent, non-undeduced-`auto` pointees when parsing a pointer or reference type.
> > > > - In SemaType, we add the default address space to non-qualified pointees when building a pointer or reference type.
> > > > - We add the default address space at the top level when when building a variable.
> > > > 
> > > > Then all of this context-specific logic where we're looking at different declarator chunks and trying to infer the relationship of the current chunk to the overall type being parsed can just go away or get pushed into a more appropriate position.
> > > Ok, it mainly works, however I still need a bit of parsing horribleness when deducing addr space of declarations with parenthesis  in `GetFullTypeForDeclarator`. This is the case for block pointers or pointers/references to arrays. It is incorrect to add address spaces on ParenType while building a pointer or references so I have to detect this as special case.
> > You can't add an address space outside a `ParenType`?  That seems odd; what problems are you seeing exactly?
> > 
> > If it's really just specific to `ParenType`, you could simply drill through them and then rebuild the `ParenType`s.
> > You can't add an address space outside a `ParenType`? That seems odd; what problems are you seeing exactly?
> 
> When we add addr space for a pointee and it's a `ParenType` the addr space should actually be added to a first non-`ParenType` but not `ParenType` itself. For example is we declare a reference to an array we want the array type to have an address space not `ParenType`.
> 
> > If it's really just specific to ParenType, you could simply drill through them and then rebuild the ParenTypes.
> 
> Ok, in the case I explained above we would have to add address space to the first non-`ParenTypes` and then rebuild all `ParenType`s. I will try that.
> 
ParenType is just a sugar node, and qualifiers on it should be treated identically to qualifiers on the inner type, just like a qualifier on a typedef name (i.e. `const int32_t`) would.  So unless you're seeing a real problem I wouldn't worry about it.

I could totally believe that it prints poorly, though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65744/new/

https://reviews.llvm.org/D65744