This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Basic parse and sema support for modifiers in order clause
ClosedPublic

Authored by sandeepkosuri on Jun 15 2022, 6:37 AM.

Details

Summary

This patch gives basic parsing and semantic support for "modifiers" of order clause introduced in OpenMP 5.1 ( section 2.11.3 )

Diff Detail

Event Timeline

sandeepkosuri created this revision.Jun 15 2022, 6:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 6:37 AM
sandeepkosuri requested review of this revision.Jun 15 2022, 6:37 AM
Herald added a project: Restricted Project. · View Herald Transcript

provide full context of the changes.

clang/include/clang/AST/OpenMPClause.h
7646

Commit as NFC change

clang/include/clang/Sema/Scope.h
483–488

Why do wee need new scope?

clang/lib/Basic/OpenMPKinds.cpp
649–654

C/C++ does not have do and workshare directives.

sandeepkosuri added inline comments.Oct 13 2022, 10:21 PM
clang/include/clang/Sema/Scope.h
483–488

I needed a new scope flag to keep track of all the new scopes created inside a region which has an associated order clause. Then I proceeded to mark all those nested scopes within 'order clause' region with this flag. I needed to do this to implement this restriction (OpenMP 5.1 - 2.11.3):

A region that corresponds to a construct with an order clause that specifies concurrent may not contain calls to the OpenMP Runtime API.

Changes in this file, in SemaOpenMP.cpp ( in Sema::ActOnOpenMPCall() ) and in Scope.cpp together form the implementation of the above restriction.

I have made some indentation changes using "git clang format" and added the context for all the files in this patch.

ABataev added inline comments.Nov 7 2022, 6:21 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
10634

Do you have the test for this error message?

clang/lib/Sema/SemaOpenMP.cpp
875

isOrderConcurrent

882

isParentOrderConcurrent

7215

Remove this

7216

I think you can drop call of StringRef constructor here, just omp_

16694

Remove this

16707

Remove this

sandeepkosuri added inline comments.Nov 9 2022, 8:53 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
10634

I forgot to add a test for this, I will do it. Thanks for noticing.

clang/lib/Sema/SemaOpenMP.cpp
875

This function is never used, So I will remove this altogether. Thanks for pointing this out.

Addressed the comments

This revision is now accepted and ready to land.Nov 11 2022, 7:33 AM

Fixed the Semantic analysis and updated the newly added LIT test case.

As I do not have commit access, can someone commit this patch, now that it passes the pre-merge tests ?

jyu2 added a subscriber: jyu2.Jan 12 2023, 1:41 PM

As I do not have commit access, can someone commit this patch, now that it passes the pre-merge tests ?

I see some tests failed after this patch. Failed only with -fopenmp-vesion=51

https://www.godbolt.org/z/3oxWTcxn7

jyu2 added a comment.Jan 17 2023, 11:10 AM

Hi @sandeepkosuri, do you plan to fix this? Thanks. Jennifer

Hi @sandeepkosuri, do you plan to fix this? Thanks. Jennifer

Hi jyu2, sorry for a late reply, and yes I will fix it. Thanks for pointing this out.

As I do not have commit access, can someone commit this patch, now that it passes the pre-merge tests ?

I see some tests failed after this patch. Failed only with -fopenmp-vesion=51

https://www.godbolt.org/z/3oxWTcxn7

Hey @jyu2 , this error is not reproducible anymore, I think the issue is solved, by someone else's patch.