This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Allow data members in interop init/use/destroy clauses
ClosedPublic

Authored by mikerice on Aug 4 2022, 4:39 PM.

Details

Summary

Previously a diagnostic was given if the expression was not strictly a
DeclRef. Now also allow use of data members inside member functions.

Diff Detail

Event Timeline

mikerice created this revision.Aug 4 2022, 4:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 4:39 PM
mikerice requested review of this revision.Aug 4 2022, 4:39 PM

Is it allowed to be a field per OpenMP standard?

Is it allowed to be a field per OpenMP standard?

The spec says: interop-var is a variable of type omp_interop_t

And defines 'variable' as: A named data storage block, for which the value can be defined and redefined during the execution of a program.

So we think yes. Users think yes. They want to use init/destroy in constructors/destructors, which seems reasonable.

Is it allowed to be a field per OpenMP standard?

The spec says: interop-var is a variable of type omp_interop_t

And defines 'variable' as: A named data storage block, for which the value can be defined and redefined during the execution of a program.

So we think yes. Users think yes. They want to use init/destroy in constructors/destructors, which seems reasonable.

I would try to get clarification from the standard committee, need to fix this in the standard, if this is not quite correct.

clang/lib/Sema/SemaOpenMP.cpp
17260

No need to introduce separate storage for the fields, reuse the existing one.

17269

Try to use getPrivateItem(), if possible.

17291

A check that the base is this?

mikerice updated this revision to Diff 450317.Aug 5 2022, 10:12 AM
mikerice marked 3 inline comments as done.

Use getPrivateItem() for interop variables.

clang/lib/Sema/SemaOpenMP.cpp
17291

getPrivateItem() does this so should be good now.

Add a test for member with base other than this?

clang/lib/Sema/SemaOpenMP.cpp
17268–17269

Can you drop const_cast by dropping const from const OMPClause *C?

clang/test/OpenMP/interop_messages.cpp
62

Previous error message was much better, need to restore it.

mikerice updated this revision to Diff 450336.Aug 5 2022, 11:29 AM
mikerice marked 2 inline comments as done.

Added optional type to diagnostic given in getPrivateItem().

ABataev added inline comments.Aug 5 2022, 11:38 AM
clang/test/OpenMP/interop_messages.cpp
62

Better to remove name from the message to make it look like before. Also, if the messageis in the context of member function, would be good to emit something like variable or data member.

mikerice updated this revision to Diff 450348.Aug 5 2022, 12:17 PM
mikerice marked an inline comment as done.

Changed message to match old message. Prints 'variable' or 'data member' depending on context.

ABataev added inline comments.Aug 5 2022, 9:46 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
10391

Maybe of the current instance or static member or something similar? data member of current class is not correct technically.

clang/lib/Sema/SemaOpenMP.cpp
5331

Will it work for static member functions?

mikerice updated this revision to Diff 450948.Aug 8 2022, 2:37 PM

To include static data members in the message and be correct in C or C++ we need three cases:

In C:
expected variable of type 'omp_interop_t'

In a C++ non-member function:
expected variable or static data member of type 'omp_interop_t'

In a C++ member function:
expected variable, static data member, or non-static data member of current class of type 'omp_interop_t'

This seems to exactly describe the situation to me. Maybe I am missing the point.
Feel free to make specific recommendations to help move this along.

To include static data members in the message and be correct in C or C++ we need three cases:

In C:
expected variable of type 'omp_interop_t'

In a C++ non-member function:
expected variable or static data member of type 'omp_interop_t'

In a C++ member function:
expected variable, static data member, or non-static data member of current class of type 'omp_interop_t'

This seems to exactly describe the situation to me. Maybe I am missing the point.
Feel free to make specific recommendations to help move this along.

You can’t use non-static member functions in static member function.

To include static data members in the message and be correct in C or C++ we need three cases:

In C:
expected variable of type 'omp_interop_t'

In a C++ non-member function:
expected variable or static data member of type 'omp_interop_t'

In a C++ member function:
expected variable, static data member, or non-static data member of current class of type 'omp_interop_t'

This seems to exactly describe the situation to me. Maybe I am missing the point.
Feel free to make specific recommendations to help move this along.

You can’t use non-static member functions in static member function.

Right so the description above should really be:

In a C++ non-member function or static member function:
expected variable or static data member of type 'omp_interop_t'

In a C++ non-static member function:
expected variable, static data member, or non-static data member of current class of type 'omp_interop_t'

I'll add testing for static member functions.

mikerice updated this revision to Diff 451164.Aug 9 2022, 8:28 AM

Added uses in static member functions to tests.

ABataev added inline comments.Aug 11 2022, 4:05 AM
clang/lib/Sema/SemaOpenMP.cpp
5330–5331

I.e. шт С++ we consider all contexts as a member function context, either static or not?

clang/test/OpenMP/interop_messages.cpp
65

We do not expect static data member here, only variable

mikerice updated this revision to Diff 451862.Aug 11 2022, 7:56 AM

Fix formatting.

clang/lib/Sema/SemaOpenMP.cpp
5330–5331

In C++ there are two cases:
S.getCurrentThisType().isNull() -> all functions except non-static member functions (static data members are allowed in here using ClassName::MemberName)

"expected variable or static data member of type 'omp_interop_t'"

!S.getCurrentThisType().isNull() -> only non-static member functions

"expected variable, static data member, or non-static data member of current class of type 'omp_interop_t'"

clang/test/OpenMP/interop_messages.cpp
65

We do not expect static data member here, only variable

Why? use(A::smember) is okay here.

ABataev accepted this revision.Aug 11 2022, 8:00 AM

LG

clang/test/OpenMP/interop_messages.cpp
65

Ah, yes, this one is allowed.

This revision is now accepted and ready to land.Aug 11 2022, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 9:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript