[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM, but you can probably undo one of my requests. :)



================
Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }
----------------
mikael wrote:
> mikael wrote:
> > rjmccall wrote:
> > > The indentation here is messed up.
> > > 
> > > You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few places.  That's currently correct, in the sense that the fast qualifiers are currently defined to be the CVR qualifiers, but the point of the distinction is that we might want to change that (and we have in fact explored that in the past).  I think you should make `FunctionTypeBits` only claim to store the fast qualifiers, which mostly just means updating a few field / accessor names and changing things like the `getCVRQualifiers()` call right above this to be `getFastQualifiers()`.
> > > 
> > > If there are places where it's convenient to assume that the CVR qualifiers are exactly the fast qualifiers, it's okay to static_assert that, but you should make sure to static_assert it in each place you assume it.
> > I'll change it, but I do have a question related to this:
> > 
> > Can we make any assumptions with the "fast qualifiers"? I'd like it if we can assume that they fit in 3 bits. Otherwise I think I also need an assertion here.
> I solved it like this in the end:
> 
> * I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to make it dependent on possible changes to the fast flags.
> * Then I put a static_assertion to guard the hastConst(), hasVolatile() and hasRestrict() methods.
Great, looks good.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+      getOrCreateInstanceMethodType(
+          CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+          FPT, U),
----------------
mikael wrote:
> rjmccall wrote:
> > Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?
> Your initial comment suggested to send in a record rather than a type. But I see now that it may make more sense to use the type directly instead. Will update it.
Oh, I actually didn't even think about the fact that the `getClass())` returns a `Type*` instead of a `CXXRecordDecl`.  Using the record decl is probably better, sorry.


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

https://reviews.llvm.org/D54862