This is an archive of the discontinued LLVM Phabricator instance.

Support for OpenMP 5.0 sec 2.12.7 - Declare Target initializer expressions
ClosedPublic

Authored by RitanyaB on Mar 20 2023, 5:52 AM.

Details

Summary

Initial support for OpenMP 5.0 declare target "as if" behavior for "initializer expressions".
If a variable with static storage duration or a function (except lambda for C++) is referenced in the initializer expression list of a variable with static storage duration that appears as a list item in a to clause on a declare target directive then the name of the referenced variable or function is treated as if it had appeared in a to clause on a declare target directive.

Diff Detail

Event Timeline

RitanyaB created this revision.Mar 20 2023, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2023, 5:52 AM
RitanyaB requested review of this revision.Mar 20 2023, 5:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
ABataev added inline comments.Mar 20 2023, 6:30 AM
clang/lib/Parse/ParseDecl.cpp
2093–2115 ↗(On Diff #506554)

It has nothing to do with parsing, sema analysis. Make it part of Sema::checkDeclIsAllowedInOpenMPTarget

2107 ↗(On Diff #506554)

Try to avoid recursion here

RitanyaB added inline comments.Mar 20 2023, 7:06 AM
clang/lib/Parse/ParseDecl.cpp
2107 ↗(On Diff #506554)

Consider the following example-

static int var1 = 10; 
static int *var2 = &var1;
#pragma omp declare target
int **ptr1=&var2;
#pragma omp end declare target

In this case, by default ptr1 gets OMPDeclareTargetDeclAttr and the function mentioned above (ParseImplicitDeclareTargetAttr) would add OMPDeclareTargetDeclAttr to var2. But, in order to add OMPDeclareTargetDeclAttr to var1, recursion would be required.

ABataev added inline comments.Mar 20 2023, 7:15 AM
clang/lib/Parse/ParseDecl.cpp
2107 ↗(On Diff #506554)

I do not ask to remove it, but use extra memory (SamllVector) to save the decls, requiring analysis, and iterate through this vector.

RitanyaB added inline comments.Mar 23 2023, 1:57 AM
clang/lib/Parse/ParseDecl.cpp
2093–2115 ↗(On Diff #506554)

The Declaration in Sema::checkDeclIsAllowedInOpenMPTarget is incomplete.

VarDecl 0x1582b278 <test3.c:6:1, col:7> col:7 ptr1 'int **'
`-OMPDeclareTargetDeclAttr 0x1582b2e0 <line:5:21> Implicit MT_To DT_Any 1
  `-<<<NULL>>>

At this point, I do not have access to the initializer expression. Any suggestions?

ABataev added inline comments.Mar 23 2023, 6:02 AM
clang/lib/Parse/ParseDecl.cpp
2093–2115 ↗(On Diff #506554)

The try to do it ActOnOpenMPDeclareTargetName

RitanyaB added inline comments.Mar 27 2023, 7:15 AM
clang/lib/Parse/ParseDecl.cpp
2093–2115 ↗(On Diff #506554)

In the absence of a declare target clause, this function is not invoked (in the examples used above, the function is never called). Do you have any suggestions?

ABataev added inline comments.Mar 28 2023, 6:07 AM
clang/lib/Parse/ParseDecl.cpp
2093–2115 ↗(On Diff #506554)

For such decls use checkDeclIsAllowedInOpenMPTarget

RitanyaB marked an inline comment as not done.Mar 28 2023, 7:14 AM
RitanyaB added inline comments.
clang/lib/Parse/ParseDecl.cpp
2093–2115 ↗(On Diff #506554)

As mentioned in my previous comment, the Decl in checkDeclIsAllowedInOpenMPTarget is not complete and I don't have access to the initializer expression hence I am unable to move my functionality into checkDeclIsAllowedInOpenMPTarget.
After ParseDeclarationAfterDeclaratorAndAttributes returns, the Decl is complete and I can process it accordingly.

ABataev added inline comments.Mar 28 2023, 7:29 AM
clang/lib/Parse/ParseDecl.cpp
2093–2115 ↗(On Diff #506554)

For decls, explicitly marked as DeclareTarget, use The try to do it ActOnOpenMPDeclareTargetName. For decls, referenced in the initializers, use checkDeclIsAllowedInOpenMPTarget.

RitanyaB updated this revision to Diff 516733.Apr 25 2023, 3:40 AM

Adding the support in SemaOpenMP.cpp.

ABataev added inline comments.Apr 25 2023, 6:37 AM
clang/lib/Sema/SemaDecl.cpp
14467–14468

if (auto *VD = dyn_cast_or_null<VarDecl>(D); LangOpts.OpenMP && VD && VD->hasAttr<OMPDeclareTargetDeclAttr>() && VD->hasGlobalStorage())

clang/lib/Sema/SemaOpenMP.cpp
23091

static void addOMP...

23092

SmallVectorImpl<Decl *> &DeclVector

23093–23100

if (!DeclRef)

return;

if (auto *VD = dyn_cast_or_null<VarDecl>(DeclRef->getDecl()) {

DeclVar->addAttr(TargetDecl->getAttr<OMPDeclareTargetDeclAttr>());
DeclVector.push_back(DeclVar);

}

23103–23114

Use StmtVisitor to implement this correctly.

23120

100 is to much, use either default value SmallVector<Decl *> DeclVector; or smaller value (4 should be enough)

23122

while (!DeclVector.empty())

23123–23124

Decl *D = DeclVector.pop_back_val();

23125–23126

Use dyn_cast instead of isa/cast

RitanyaB updated this revision to Diff 520729.May 9 2023, 9:30 AM
ABataev added inline comments.May 9 2023, 9:35 AM
clang/lib/Sema/SemaDecl.cpp
14467

В is already checked that is not nullptr, so just a dyn_cast should be enough

clang/lib/Sema/SemaOpenMP.cpp
23093–23094

Why public?

23097

const auto *VD

23098

Use VD instead of this and drop const

23099–23100

Drop this and extra parens

23106–23109

Just Visit(*it)?

RitanyaB updated this revision to Diff 521014.May 10 2023, 9:20 AM
RitanyaB added inline comments.May 10 2023, 9:25 AM
clang/lib/Sema/SemaOpenMP.cpp
23093–23094

As the data members are accessed from outside the class (in ActOnOpenMPImplicitDeclareTarget function), I have made them public.

23106–23109

Or I can call VisitDeclRefExpr directly. Would that be more suitable?

ABataev added inline comments.May 10 2023, 9:30 AM
clang/lib/Sema/SemaOpenMP.cpp
23093–23094

If you need them, implement getters for them and make them private.

23105–23106

Why just a regular Visit does not work here? Plus, isa + dyn_cast is weird.

RitanyaB updated this revision to Diff 521902.May 13 2023, 6:58 AM
ABataev added inline comments.May 15 2023, 7:04 AM
clang/include/clang/Sema/Sema.h
11322

Do I understand correctly, that the variable itself is marked explicitly, but the initializer shall be marked implicitly? If so, please fix the function name

clang/lib/Sema/SemaOpenMP.cpp
23096–23097

Remove this->, where not required.

23098
  1. Wrong function name format.
  2. Do you really need to make it public? Hide as much as possible.
23101

Same, as for Push

23125–23139

Better to hide this in GlobalDeclRefChecker

RitanyaB updated this revision to Diff 524290.May 22 2023, 7:08 AM
ABataev added inline comments.May 22 2023, 7:15 AM
clang/lib/Sema/SemaDecl.cpp
14469–14471

Remove braces

clang/lib/Sema/SemaOpenMP.cpp
23090

Please, add comments, describing the class and its functionality

23094

setTargetDecl

23095
  1. checkDeclVector
  2. const member function
23097

const member function

23103

Remove this->

23112
  1. Better to make it static member function.
  2. Follow the rules for function names.
RitanyaB updated this revision to Diff 525531.May 25 2023, 4:00 AM
ABataev added inline comments.May 25 2023, 9:52 AM
clang/lib/Sema/SemaOpenMP.cpp
23108–23109

for (const Expr *Child : Ex->children())

23125–23130

Shall just something like this work here:

if (Ex)
  Visit(Ex);
RitanyaB updated this revision to Diff 526377.May 28 2023, 9:55 PM
ABataev added inline comments.May 30 2023, 6:14 AM
clang/lib/Sema/SemaOpenMP.cpp
23123

Better to expand auto here

23124

Formatting?

RitanyaB updated this revision to Diff 526665.May 30 2023, 9:09 AM
ABataev added inline comments.May 30 2023, 9:13 AM
clang/lib/Sema/SemaOpenMP.cpp
23093

Use just a StmtVisitor, if you still dropping const modifier.

23101

remove const_cast

23118

You don't need to use dyn_cast here, use just cast

RitanyaB updated this revision to Diff 526694.May 30 2023, 10:12 AM
ABataev added inline comments.May 30 2023, 10:17 AM
clang/lib/Sema/SemaOpenMP.cpp
23114

Fix the strange symbol in the comment

23116

Function names should be verb phrases (as they represent actions), and command-like function should be imperative. (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

RitanyaB updated this revision to Diff 526915.May 30 2023, 11:03 PM
This revision is now accepted and ready to land.May 31 2023, 6:24 AM
This revision was landed with ongoing or failed builds.Jun 1 2023, 3:29 AM
This revision was automatically updated to reflect the committed changes.
DavidSpickett added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
23135

FYI I fixed an unused var warning here: https://github.com/llvm/llvm-project/commit/f3ca99a87c566fb5e910071f4cbb474ddb4e7f37

Potentially you could pass the result of the cast into declareTargetInitializer but I don't know enough about the code to change that too.