Page MenuHomePhabricator

[OpenCL] Emit function-scope variable in constant address space as static variable

Authored by yaxunl on May 8 2017, 1:18 PM.

Diff Detail


Event Timeline

yaxunl created this revision.May 8 2017, 1:18 PM
rjmccall added inline comments.May 9 2017, 3:23 PM
7129 ↗(On Diff #98197)

Seeing criteria like this, and a lot of the other things you've modified, makes me feel like we should just have a VarDecl::getStorageDuration().

10286 ↗(On Diff #98197)

Should this rule apply even in C++ mode? I can't remember if there are any OpenCL/C++ hybrids.

yaxunl marked an inline comment as done.May 10 2017, 6:33 AM
yaxunl added inline comments.
10286 ↗(On Diff #98197)

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.

yaxunl updated this revision to Diff 98463.May 10 2017, 8:22 AM
yaxunl marked 2 inline comments as done.

Use getStorageDuration to decide how to emit a function-scope variable.

rjmccall added inline comments.May 10 2017, 12:28 PM
158 ↗(On Diff #98463)

Please rearrange this function so that the D.hasExternalStorage() case below comes first; that will then simplify this condition.

10286 ↗(On Diff #98197)

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)) {
yaxunl marked 3 inline comments as done.May 10 2017, 12:36 PM
yaxunl added inline comments.
10286 ↗(On Diff #98197)

Agree that even OpenCL C++ is unable to lazy initialise function-scope var in constant addr space. Will do.

yaxunl updated this revision to Diff 98510.May 10 2017, 1:06 PM
yaxunl marked an inline comment as done.

Revised by John's comments.

rjmccall accepted this revision.May 10 2017, 3:37 PM

LGTM, thanks!

This revision is now accepted and ready to land.May 10 2017, 3:37 PM
Anastasia accepted this revision.May 12 2017, 6:52 AM


10286 ↗(On Diff #98197)

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.

yaxunl added inline comments.May 12 2017, 1:49 PM
10286 ↗(On Diff #98197)

I will delete the comment about OpenCL C++ when committing.

rjmccall added inline comments.May 13 2017, 1:54 AM
10286 ↗(On Diff #98197)

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.

yaxunl added inline comments.May 14 2017, 11:25 AM
10286 ↗(On Diff #98197)

I double checked OpenCL 2.2 C++ language spec and did some experiment with the khronos implementation:

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.

This revision was automatically updated to reflect the committed changes.
Anastasia added inline comments.May 15 2017, 10:38 AM
10286 ↗(On Diff #98197)

Btw, it does seem to be adding OpenCLCPlusPlus to LangOpts.

rjmccall added inline comments.May 15 2017, 12:45 PM
10286 ↗(On Diff #98197)

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.