[PATCH] D150394: [OpenMP 5.2] Deprecate MINUS (-) operator on reduction clauses

[Fazlay Rabbi via Phabricator via cfe-commits]

mdfazlay closed this revision.
mdfazlay added a comment.

Commit ID - c657363 



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

https://reviews.llvm.org/D150394

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

[PATCH] D150394: [OpenMP 5.2] Deprecate MINUS (-) operator on reduction clauses

[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




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10487-10488
   "a reduction list item with incomplete type %0">;
+def err_omp_reduction_minus_type : Error<
+  "a reduction list item with minus(-) operator is not supported">;
+def warn_omp_minus_in_reduction_deprecated : Warning<

mdfazlay wrote:
> ABataev wrote:
> > We already have message for unsupported reduction identifier. Better to 
> > modify switch in static bool actOnOMPReductionKindClause function and check 
> > if OpenMP > 52, set BOK to BO_Comma, if incoming OOK is OO_Minus. In this 
> > case it will be automatically diagnosed for OpenMP>52 as not allowed 
> > reduction identifier.
> BTW, I think we need to remove minus (-) from the incorrect reduction 
> identifier error message once OpenMP 6.0 becomes official. Please take a look 
> at the following output:
> 
> ```
> $ cat test.c
> void foo() {
>   int a = 0 ;
> #pragma omp parallel reduction(-:a)
>   ;
> }
> ```
> ```
> > clang -fopenmp -fopenmp-version=60 test.c -c
> test.c:3:32: error: incorrect reduction identifier, expected one of '+', '-', 
> '*', '&', '|', '^', '&&', '||', 'min' or 'max' or declare reduction for type 
> 'int'
> #pragma omp parallel reduction(-:a)
>^
> 1 error generated.
> ```
Yes, need to create versioned message:prior 6.0 and after 6.0.


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

https://reviews.llvm.org/D150394

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

[PATCH] D150394: [OpenMP 5.2] Deprecate MINUS (-) operator on reduction clauses

[Fazlay Rabbi via Phabricator via cfe-commits]

mdfazlay added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10487-10488
   "a reduction list item with incomplete type %0">;
+def err_omp_reduction_minus_type : Error<
+  "a reduction list item with minus(-) operator is not supported">;
+def warn_omp_minus_in_reduction_deprecated : Warning<

ABataev wrote:
> We already have message for unsupported reduction identifier. Better to 
> modify switch in static bool actOnOMPReductionKindClause function and check 
> if OpenMP > 52, set BOK to BO_Comma, if incoming OOK is OO_Minus. In this 
> case it will be automatically diagnosed for OpenMP>52 as not allowed 
> reduction identifier.
BTW, I think we need to remove minus (-) from the incorrect reduction 
identifier error message once OpenMP 6.0 becomes official. Please take a look 
at the following output:

```
$ cat test.c
void foo() {
  int a = 0 ;
#pragma omp parallel reduction(-:a)
  ;
}
```
```
> clang -fopenmp -fopenmp-version=60 test.c -c
test.c:3:32: error: incorrect reduction identifier, expected one of '+', '-', 
'*', '&', '|', '^', '&&', '||', 'min' or 'max' or declare reduction for type 
'int'
#pragma omp parallel reduction(-:a)
   ^
1 error generated.
```


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

https://reviews.llvm.org/D150394

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

[PATCH] D150394: [OpenMP 5.2] Deprecate MINUS (-) operator on reduction clauses

[Fazlay Rabbi via Phabricator via cfe-commits]

mdfazlay added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10490
+def warn_omp_minus_in_reduction_deprecated : Warning<
+  "minus(-) operator for reductions is deprecated; use an user defined 
reduction instead">,
+  InGroup;

ABataev wrote:
> mdfazlay wrote:
> > ABataev wrote:
> > > `use + or user defined reudction instead`? And better make it a fixme 
> > > note.
> > @ABataev, Do you want me to add a warning and a note at the same location? 
> > Or Changing the warning message to `minus(-) operator for reductions is 
> > deprecated; use + or user defined reduction instead` is just fine? Please 
> > let me know which one you prefer.
> Would be good to have both. But since it is a temp warning, I'm fine with 
> your message
I have addressed your comments. Please let me know if anything needs to be 
revised. 


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

https://reviews.llvm.org/D150394

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

[PATCH] D150394: [OpenMP 5.2] Deprecate MINUS (-) operator on reduction clauses

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10490
+def warn_omp_minus_in_reduction_deprecated : Warning<
+  "minus(-) operator for reductions is deprecated; use an user defined 
reduction instead">,
+  InGroup;

mdfazlay wrote:
> ABataev wrote:
> > `use + or user defined reudction instead`? And better make it a fixme note.
> @ABataev, Do you want me to add a warning and a note at the same location? Or 
> Changing the warning message to `minus(-) operator for reductions is 
> deprecated; use + or user defined reduction instead` is just fine? Please let 
> me know which one you prefer.
Would be good to have both. But since it is a temp warning, I'm fine with your 
message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150394

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

[PATCH] D150394: [OpenMP 5.2] Deprecate MINUS (-) operator on reduction clauses

[Fazlay Rabbi via Phabricator via cfe-commits]

mdfazlay added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10490
+def warn_omp_minus_in_reduction_deprecated : Warning<
+  "minus(-) operator for reductions is deprecated; use an user defined 
reduction instead">,
+  InGroup;

ABataev wrote:
> `use + or user defined reudction instead`? And better make it a fixme note.
@ABataev, Do you want me to add a warning and a note at the same location? Or 
Changing the warning message to `minus(-) operator for reductions is 
deprecated; use + or user defined reduction instead` is just fine? Please let 
me know which one you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150394

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

[PATCH] D150394: [OpenMP 5.2] Deprecate MINUS (-) operator on reduction clauses

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10487-10488
   "a reduction list item with incomplete type %0">;
+def err_omp_reduction_minus_type : Error<
+  "a reduction list item with minus(-) operator is not supported">;
+def warn_omp_minus_in_reduction_deprecated : Warning<

We already have message for unsupported reduction identifier. Better to modify 
switch in static bool actOnOMPReductionKindClause function and check if OpenMP 
> 52, set BOK to BO_Comma, if incoming OOK is OO_Minus. In this case it will be 
automatically diagnosed for OpenMP>52 as not allowed reduction identifier.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10490
+def warn_omp_minus_in_reduction_deprecated : Warning<
+  "minus(-) operator for reductions is deprecated; use an user defined 
reduction instead">,
+  InGroup;

`use + or user defined reudction instead`? And better make it a fixme note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150394

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