rjmccall added inline comments.
================
Comment at: lib/Sema/SemaDecl.cpp:10286
+ // these variables must be a compile time constant.
+ VDecl->getType().getAddressSpace() == LangAS::opencl_constant)
CheckForConstantInitializer(Init, DclT);
----------------
Anastasia wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > yaxunl wrote:
> > > > > > rjmccall wrote:
> > > > > > > yaxunl wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > Should this rule apply even in C++ mode? I can't remember if
> > > > > > > > > there are any OpenCL/C++ hybrids.
> > > > > > > > No. This rule (static local variable needs to be initialised
> > > > > > > > with compile-time constant) only applies to C and OpenCL.
> > > > > > > > In C++ static local variable can be initialised with
> > > > > > > > non-compile-time constant, in which case Clang will emit atomic
> > > > > > > > instructions to make sure it is only initialised once.
> > > > > > > >
> > > > > > > > Currently OpenCL 2.2 defines OpenCL C++ but clang does not
> > > > > > > > support it.
> > > > > > > Yes, I understand that C++ generally allows static locals to be
> > > > > > > lazily initialized, and that that rule would (probably) still
> > > > > > > apply to ordinary static locals in OpenCL C++. However, I would
> > > > > > > expect that OpenCL C++ rule is that __constant local variables
> > > > > > > still need to be statically initialized, because there's no
> > > > > > > plausible way in the OpenCL implementation model to actually put
> > > > > > > lazily-initialized variables in the constant address space.
> > > > > > > Assuming that's correct, then I would recommend reworking this
> > > > > > > whole chain of conditions to:
> > > > > > >
> > > > > > > // Don't check the initializer if the declaration is malformed.
> > > > > > > if (VDecl->isInvalidDecl()) {
> > > > > > > // do nothing
> > > > > > >
> > > > > > > // OpenCL __constant locals must be constant-initialized, even
> > > > > > > in OpenCL C++.
> > > > > > > } else if (VDecl->getType().getAddressSpace() ==
> > > > > > > LangAS::opencl_constant) {
> > > > > > > CheckForConstantInitializer(Init, DclT);
> > > > > > >
> > > > > > > // Otherwise, C++ does not restrict the initializer.
> > > > > > > } else if (getLangOpts().CPlusPlus) {
> > > > > > > // do nothing
> > > > > > >
> > > > > > > // C99 6.7.8p4: All the expressions in an initializer for an
> > > > > > > object that has
> > > > > > > // static storage duration shall be constant expressions or
> > > > > > > string literals.
> > > > > > > } else if (VDecl->getStorageClass() == SC_Static) {
> > > > > > > CheckForConstantInitializer(Init, DclT);
> > > > > > >
> > > > > > > // C89 is stricter than C99 for aggregate initializers.
> > > > > > > // C89 6.5.7p3: All the expressions [...] in an initializer list
> > > > > > > // for an object that has aggregate or union type shall be
> > > > > > > // constant expressions.
> > > > > > > } else if (!getLangOpts().C99 &&
> > > > > > > VDecl->getType()->isAggregateType() && isa<InitListExpr>(Init)) {
> > > > > > > Expr *Culprit;
> > > > > > > if (!Init->isConstantInitializer(Context, false, &Culprit)) {
> > > > > > > ...
> > > > > > > }
> > > > > > > }
> > > > > > Agree that even OpenCL C++ is unable to lazy initialise
> > > > > > function-scope var in constant addr space. Will do.
> > > > > I think the original way would be much simpler to read and understand
> > > > > though.
> > > > >
> > > > > To be honest I wouldn't complicate things now for the feature we
> > > > > don't support. I feel OpenCL C++ should be represented as a separate
> > > > > LangOpt since there are some places that will require special
> > > > > handling due to deviation from C++. I would rather extend things
> > > > > later in more systematic way.
> > > > I will delete the comment about OpenCL C++ when committing.
> > > I disagree. Simple chains like this are almost always superior to
> > > building up complex logical conditions: the priorities between conditions
> > > are clearer (such as the interaction between __constant and C++ here),
> > > each condition can be conveniently documented without having to add
> > > comments to the middle of an expression, and there's less need to build
> > > up (A && !B) conditions just to make sure that cases are routed to the
> > > right place. If the body of a clause is complex, it's usually a good
> > > idea to extract it into a separate function anyway, as has already been
> > > done here with CheckForConstantInitializer.
> > >
> > > Deleting the comment about OpenCL C++ seems silly. The comment is
> > > correct and explains why the clauses need to be ordered the way they are,
> > > and someone implementing OpenCL C++ support later will not think to add
> > > it back.
> > >
> > > Please trust me that you would not want to use a different LangOpt for
> > > OpenCL C++. OpenCL C++ may feel a little different from normal C++ to a
> > > user, but for the compiler its deviations are tiny compared to the number
> > > of ways in which C++ deviates from C. The major C++ features that really
> > > cut across the frontend are all still there: templates, references, type
> > > members, function overloading, operator overloading, totally different
> > > lookup rules, etc.
> > I double checked OpenCL 2.2 C++ language spec and did some experiment with
> > the khronos implementation:
> >
> > https://www.khronos.org/registry/OpenCL/specs/opencl-2.2-cplusplus.pdf
> >
> > https://github.com/KhronosGroup/libclcxx
> >
> > I think you are right about the constant address space in OpenCL C++.
> > Although OpenCL C++ uses class templates to implement address space, it
> > does use `__constant` to implement constant class template internally, and
> > supports `__constant` address space qualifier. Therefore the comment about
> > OpenCL C++ should be correct.
> >
> Btw, it does seem to be adding `OpenCLCPlusPlus` to `LangOpts`.
Well, they can do whatever they want in a fork. As long as they're also
setting CPlusPlus it's not a big deal. But I suspect that overall it's quite a
bit like Objective-C++, where it makes a lot more sense to keep them as
separate flags and just occasionally add C++-specific code to an ObjC code path
or vice-versa.
Repository:
rL LLVM
https://reviews.llvm.org/D32977
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
