[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2023-02-08 Thread Sandeep via Phabricator via cfe-commits 
sandeepkosuri added a comment.

In D127855#4048642 , @jyu2 wrote:

> In D127855#3956014 , @sandeepkosuri 
> wrote:
>
>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2023-01-17 Thread Sandeep via Phabricator via cfe-commits 
sandeepkosuri added a comment.

In D127855#4059776 , @jyu2 wrote:

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2023-01-17 Thread Jennifer Yu via Phabricator via cfe-commits 
jyu2 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2023-01-12 Thread Jennifer Yu via Phabricator via cfe-commits 
jyu2 added a comment.

In D127855#3956014 , @sandeepkosuri 
wrote:

> 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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2022-11-28 Thread Sandeep via Phabricator via cfe-commits 
sandeepkosuri added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2022-11-11 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2022-11-09 Thread Sandeep via Phabricator via cfe-commits 
sandeepkosuri added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10634
   "OpenMP constructs may not be nested inside an atomic region">;
+def err_omp_prohibited_region_order
+: Error<"construct %0 not allowed in a region associated with a directive "

ABataev wrote:
> Do you have the test for this error message?
I forgot to add a test for this, I will do it. Thanks for noticing.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:875
+  /// false - otherwise.
+  bool HasOrderConcurrent() const {
+if (const SharingMapTy *Top = getTopOfStackOrNull())

ABataev wrote:
> `isOrderConcurrent`
This function is never used, So I will remove this altogether. Thanks for 
pointing this out.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2022-11-07 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10634
   "OpenMP constructs may not be nested inside an atomic region">;
+def err_omp_prohibited_region_order
+: Error<"construct %0 not allowed in a region associated with a directive "

Do you have the test for this error message?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:875
+  /// false - otherwise.
+  bool HasOrderConcurrent() const {
+if (const SharingMapTy *Top = getTopOfStackOrNull())

`isOrderConcurrent`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:882
+  /// 'order' clause), false - otherwise.
+  bool ParentHasOrderConcurrent() const {
+if (const SharingMapTy *Parent = getSecondOnStackOrNull())

`isParentOrderConcurrent`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:7215
 
+  if (this->LangOpts.OpenMP >= 51 && CalleeFnDecl->getIdentifier() &&
+  CalleeFnDecl->getName().startswith_insensitive(StringRef("omp_"))) {

Remove `this`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:7216
+  if (this->LangOpts.OpenMP >= 51 && CalleeFnDecl->getIdentifier() &&
+  CalleeFnDecl->getName().startswith_insensitive(StringRef("omp_"))) {
+// checking for any calls inside an Order region

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16694
+  if (Kind != OMPC_ORDER_concurrent ||
+  (this->LangOpts.OpenMP < 51 && MLoc.isValid())) {
+// Kind should be concurrent,

Remove `this`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16707
   }
-  return new (Context)
-  OMPOrderClause(Kind, KindKwLoc, StartLoc, LParenLoc, EndLoc);
+  if (this->LangOpts.OpenMP >= 51) {
+if (Modifier == OMPC_ORDER_MODIFIER_unknown && MLoc.isValid()) {

Remove `this`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2022-10-13 Thread Sandeep via Phabricator via cfe-commits 
sandeepkosuri added inline comments.



Comment at: clang/include/clang/Sema/Scope.h:483-488
+  /// Determine whether this scope is some OpenMP directive with
+  /// order clause which specifies concurrent scope.
+  bool isOpenMPOrderClauseScope() const{
+return getFlags() & Scope::OpenMPOrderClauseScope;
+  }
+

ABataev wrote:
> Why do wee need new scope?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127855: [OpenMP] Basic parse and sema support for modifiers in order clause

2022-06-16 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added a comment.

provide full context of the changes.




Comment at: clang/include/clang/AST/OpenMPClause.h:7646
 
-  /// A kind of the 'default' clause.
+  /// A kind of the 'order' clause.
   OpenMPOrderClauseKind Kind = OMPC_ORDER_unknown;

Commit as NFC change



Comment at: clang/include/clang/Sema/Scope.h:483-488
+  /// Determine whether this scope is some OpenMP directive with
+  /// order clause which specifies concurrent scope.
+  bool isOpenMPOrderClauseScope() const{
+return getFlags() & Scope::OpenMPOrderClauseScope;
+  }
+

Why do wee need new scope?



Comment at: clang/lib/Basic/OpenMPKinds.cpp:649-654
+  return DKind == OMPD_parallel_do || DKind == OMPD_parallel_do_simd ||
+ DKind == OMPD_parallel_for || DKind == OMPD_parallel_for_simd ||
+ DKind == OMPD_parallel_master ||
+ DKind == OMPD_parallel_master_taskloop ||
+ DKind == OMPD_parallel_master_taskloop_simd ||
+ DKind == OMPD_parallel_workshare || DKind == OMPD_parallel_sections;

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127855/new/

https://reviews.llvm.org/D127855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits