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

This revision was automatically updated to reflect the committed changes. Closed by commit rGa29aa4710620: [OpenCL] Move addr space deduction to Sema. (authored by Anastasia). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D65744?vs=227841&id=231218#toc Reposi

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

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDecl.cpp:6721 + if (getLangOpts().OpenCL) { + Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccal

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

Anastasia added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6721 + if (getLangOpts().OpenCL) { + rjmccall wrote: > Anastasia wrote: > > rjmccall wrote: > > > Since you're moving this code anyway, can this be split into its own > > > function? I'

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

Anastasia updated this revision to Diff 227841. Anastasia marked an inline comment as done. Anastasia added a comment. - Factored OpenCL diagnostics out in a separate helper function - Removed special case for ParenType CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65744/new/ https://re

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

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 actual

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

Anastasia marked 2 inline comments as done. Anastasia added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6721 + if (getLangOpts().OpenCL) { + rjmccall wrote: > Since you're moving this code anyway, can this be split into its own > function? I'm n

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:5998 +QualType Type = Var->getType(); +if (Type->isSamplerT() || Type->isVoidType()) + return; Anastasia wrote: > I don't seem to need a check for dependent or auto types because

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

Anastasia marked 2 inline comments as done. Anastasia added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:5998 +QualType Type = Var->getType(); +if (Type->isSamplerT() || Type->isVoidType()) + return; I don't seem to need a check for depen

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

Anastasia updated this revision to Diff 219903. Anastasia added a comment. - Move addr space deduction to a later phase. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65744/new/ https://reviews.llvm.org/D65744 Files: clang/include/clang/AST/Type.h clang/include/clang/Sema/Sema.h

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

rjmccall added inline comments. 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: Okay, I understand why you're d

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

Anastasia updated this revision to Diff 218661. Anastasia added a comment. - Added forgotten test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65744/new/ https://reviews.llvm.org/D65744 Files: clang/include/clang/AST/Type.h clang/lib/AST/Expr.cpp clang/lib/Sema/SemaType.cpp cl

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

Anastasia added a comment. In D65744#1639175 , @mantognini wrote: > I think this looks good. Maybe the tests should be extended to test `auto` as > function return type, and if there's some special handling around > `decltype(auto)`, then it should be te

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

Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: include/clang/AST/Type.h:6509 + return isa(CanonicalType); +} + rjmccall wrote: > Hmm. So this method, confusingly, will not return true for a deduced `auto`, > unless the

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

Anastasia added a comment. In D65744#1653081 , @rjmccall wrote: > In D65744#1652355 , @Anastasia wrote: > > > I don't think this is likely to change. Are you suggesting to move the > > deduction logic for pointee o

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

Anastasia updated this revision to Diff 218639. Anastasia added a comment. Moved addr space of pointee inference into Build* helpers of Sema. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65744/new/ https://reviews.llvm.org/D65744 Files: clang/include/clang/AST/Type.h clang/lib/AST

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

rjmccall added a comment. In D65744#1652355 , @Anastasia wrote: > I don't think this is likely to change. Are you suggesting to move the > deduction logic for pointee of pointers, references and block pointers into > ASTContext helper that creates a poin

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

Anastasia marked an inline comment as done. Anastasia added a comment. missing test case 1 auto* accumulator() 2 { 3 __global int * glptr; 4 return glptr; 5 } 6 int i; 7 auto* ai = &i; 8 int* ii = &i; In D65744#1646348 , @rjmccall wrot

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

rjmccall added a comment. In D65744#1629055 , @Anastasia wrote: > In D65744#1627589 , @rjmccall wrote: > > > I see. Is the deduction rule recursive or something, where a pointer to > > pointer is inferred to point

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

mantognini added a comment. I think this looks good. Maybe the tests should be extended to test `auto` as function return type, and if there's some special handling around `decltype(auto)`, then it should be tested too, but I'm not sure it's actually needed here. What do you think? =

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

Anastasia added a comment. In D65744#1627589 , @rjmccall wrote: > I see. Is the deduction rule recursive or something, where a pointer to > pointer is inferred to point to the same address space as the pointee? It is recursive indeed and we currently d

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

rjmccall added a comment. I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee? I still don't understand why pointers and references are handled differently here, instead of having the rule be "don't infe

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

Anastasia added a comment. In D65744#1626563 , @rjmccall wrote: > Isn't the general rule for template argument deduction (which this devolves > to) just to ignore top-level qualifiers? And then you can substitute in the > substituted type and end up wit

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

rjmccall added a comment. Isn't the general rule for template argument deduction (which this devolves to) just to ignore top-level qualifiers? And then you can substitute in the substituted type and end up with a properly qualified type for the parameter / variable, and you can add extra quali

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

Anastasia created this revision. Anastasia added a reviewer: rjmccall. Herald added subscribers: ebevhan, yaxunl. A regression was introduced in D64400 because auto is using the same logic as templates. However, deduction of addr spaces wasn't working correctly