diff mbox series

OpenMP: Fixes for omp critical + hint

Message ID 7163bb7a-6272-331e-e830-ef8938246cc6@mentor.com
State New
Headers show
Series OpenMP: Fixes for omp critical + hint | expand

Commit Message

Tobias Burnus July 21, 2020, 11:11 a.m. UTC
Changes:
* OpenMP requires a name with the hint clause, but not if the
   expression evaluates to zero == omp_sync_hint_none.
   The "but" was not implemented.
* C++ lacked some of the C checks
* I added a >= 0 check; permitted values are between 0 and 10
   plus some '|' or '+' combinations, hence, one could check more.
* Fortran: omp_lib lacked the named constants
* Fortran: a bunch of checks were missing + trans-omp.c handling.

OK for the trunk?

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

Comments

Jakub Jelinek July 21, 2020, 11:25 a.m. UTC | #1
On Tue, Jul 21, 2020 at 01:11:31PM +0200, Tobias Burnus wrote:
> --- a/gcc/c-family/c-omp.c
> +++ b/gcc/c-family/c-omp.c
> @@ -106,6 +106,18 @@ c_finish_omp_taskgroup (location_t loc, tree body, tree clauses)
>  tree
>  c_finish_omp_critical (location_t loc, tree body, tree name, tree clauses)
>  {
> +  gcc_assert (!clauses || OMP_CLAUSE_CODE (clauses) == OMP_CLAUSE_HINT);
> +  if (name == NULL_TREE && clauses != NULL_TREE
> +      && INTEGRAL_TYPE_P (TREE_TYPE (OMP_CLAUSE_HINT_EXPR (clauses)))
> +      && tree_fits_shwi_p (OMP_CLAUSE_HINT_EXPR (clauses))
> +      && tree_to_shwi (OMP_CLAUSE_HINT_EXPR (clauses)) != 0)

Use
  if (name == NULL_TREE
      && clauses != NULL_TREE
      && integer_nonzerop (OMP_CLAUSE_HINT_EXPR (clauses)))
instead?

>        if (!INTEGRAL_TYPE_P (TREE_TYPE (t))
> -	  || TREE_CODE (t) != INTEGER_CST)
> +	  || TREE_CODE (t) != INTEGER_CST
> +	  || tree_to_shwi (t) < 0)

This will ICE if using e.g. hint (-1ULL).
Better use 
	  || tree_int_cst_sgn (t) == -1)
?
> +      t = fold_non_dependent_expr (t);
> +      if (!value_dependent_expression_p (t)
> +	  && (!INTEGRAL_TYPE_P (TREE_TYPE (t))
> +	      || !tree_fits_shwi_p (t)
> +	      || tree_to_shwi (t) < 0))

See above.

	Jakub
Tobias Burnus July 21, 2020, 12:05 p.m. UTC | #2
On 7/21/20 1:25 PM, Jakub Jelinek via Fortran wrote:

>>   c_finish_omp_critical (location_t loc, tree body, tree name, tree clauses)
>>   {
>> +  gcc_assert (!clauses || OMP_CLAUSE_CODE (clauses) == OMP_CLAUSE_HINT);
>> +  if (name == NULL_TREE && clauses != NULL_TREE
>> +      && INTEGRAL_TYPE_P (TREE_TYPE (OMP_CLAUSE_HINT_EXPR (clauses)))
>> +      && tree_fits_shwi_p (OMP_CLAUSE_HINT_EXPR (clauses))
>> +      && tree_to_shwi (OMP_CLAUSE_HINT_EXPR (clauses)) != 0)
> Use
>    if (name == NULL_TREE
>        && clauses != NULL_TREE
>        && integer_nonzerop (OMP_CLAUSE_HINT_EXPR (clauses)))
> instead?

This code is both called by C and C++ – and for C++. And I would like
to permit "hint(N)" where N is a template parameter.

Hence, I added another:
   '&& TREE_CODE (OMP_CLAUSE_HINT_EXPR (clauses)) == INTEGER_CST'
to your proposal.

Thanks for your suggestions!

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Jakub Jelinek July 21, 2020, 12:18 p.m. UTC | #3
On Tue, Jul 21, 2020 at 02:05:36PM +0200, Tobias Burnus wrote:
> This code is both called by C and C++ – and for C++. And I would like
> to permit "hint(N)" where N is a template parameter.
> 
> Hence, I added another:
>   '&& TREE_CODE (OMP_CLAUSE_HINT_EXPR (clauses)) == INTEGER_CST'
> to your proposal.

Oops, but that just shows that we don't really handle critical in such case
correctly, because nothing will then try to verify the value after
instantiation.

The usual way how to handle instantiation is if something is type (or
value) dependent (depending on what it is), we punt early on some checks and
usually just always for processing_template_decl or perhaps just when
something is dependent create some temporary tree and then redo the checks
during instantiation.

While if c_finish_omp_critical is careful not to do anything that would
be upset if something in it contains C++ FE specific trees and is dependent
we can perhaps use it, I think we need to handle OMP_CRITICAL differently in
pt.c then from what it does now, prepare arguments for c_finish_omp_critical
and call it again.

Consider:
template <int N>
void
foo ()
{
  #pragma omp critical hint (N)
  ;
}
// So no diagnostics above during processing_template_decl
void
bar ()
{
  foo <0> (); // OK
  foo <1> (); // This should be diagnosed during instantiation
}

	Jakub
Tobias Burnus July 21, 2020, 3:43 p.m. UTC | #4
On 7/21/20 2:18 PM, Jakub Jelinek wrote:
> On Tue, Jul 21, 2020 at 02:05:36PM +0200, Tobias Burnus wrote:
>> This code is both called by C and C++ – and for C++. And I would like
>> to permit "hint(N)" where N is a template parameter.
>>
>> Hence, I added another:
>>    '&& TREE_CODE (OMP_CLAUSE_HINT_EXPR (clauses)) == INTEGER_CST'
>> to your proposal.
> Oops, but that just shows that we don't really handle critical in such case
> correctly, because nothing will then try to verify the value after
> instantiation.

In principle, the compiler is not required to diagnose all invalid code ...
In any case, I have now implemented it now also in pt.c.

Cheers,

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Jakub Jelinek July 21, 2020, 3:52 p.m. UTC | #5
On Tue, Jul 21, 2020 at 05:43:00PM +0200, Tobias Burnus wrote:
> --- a/gcc/c-family/c-omp.c
> +++ b/gcc/c-family/c-omp.c
> @@ -106,6 +106,18 @@ c_finish_omp_taskgroup (location_t loc, tree body, tree clauses)
>  tree
>  c_finish_omp_critical (location_t loc, tree body, tree name, tree clauses)
>  {
> +  gcc_assert (!clauses || OMP_CLAUSE_CODE (clauses) == OMP_CLAUSE_HINT);
> +  if (name == NULL_TREE
> +      && clauses != NULL_TREE
> +      && TREE_CODE (OMP_CLAUSE_HINT_EXPR (clauses)) == INTEGER_CST
> +      && integer_nonzerop (OMP_CLAUSE_HINT_EXPR (clauses)))

I still don't see the point of the INTEGER_CST, integer_nonzerop does that
(well, it allows also COMPLEX_CST, but the clauses checking should complain
if the expression is complex).

So, ok for trunk with that line removed.

Thanks and sorry for not getting everything at once.

	Jakub
Thomas Schwinge July 22, 2020, 9:09 a.m. UTC | #6
Hi Tobias!

On 2020-07-21T17:43:00+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 7/21/20 2:18 PM, Jakub Jelinek wrote:
>> [...] shows that we don't really handle critical in such case
>> correctly, because nothing will then try to verify the value after
>> instantiation.
>
> In principle, the compiler is not required to diagnose all invalid code ...

Sure, but we should at least make an attempt to implement consistent
checking for reasonably similar cases that we know about requiring
separate/similar handling, such as non-templated vs. templated C++.

> In any case, I have now implemented it now also in pt.c.

Thanks.


> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/critical-hint-1.c
> @@ -0,0 +1,47 @@
> +#include <omp.h>

For build-tree testing, that'll pick up the *system* 'omp.h', thus:

    FAIL: c-c++-common/gomp/critical-hint-1.c (test for excess errors)
    Excess errors:
    [...]/c-c++-common/gomp/critical-hint-1.c:10:33: error: 'omp_sync_hint_none' undeclared (first use in this function); did you mean 'omp_lock_hint_none'?
    [...]

> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/critical-hint-2.c
> @@ -0,0 +1,36 @@
> +/* { dg-additional-options "-fdump-tree-original" } */
> +#include <omp.h>

Likewise.

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/critical-hint-1.f90
> @@ -0,0 +1,94 @@
> +subroutine example_criticial ()
> +  use omp_lib

Similar:

    FAIL: gfortran.dg/gomp/critical-hint-1.f90   -O  (test for excess errors)
    Excess errors:
    [...]/gfortran.dg/gomp/critical-hint-1.f90:2:7: Fatal Error: Cannot open module file 'omp_lib.mod' for reading at (1): No such file or directory

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/critical-hint-2.f90
> @@ -0,0 +1,65 @@
> +! { dg-additional-options "-fdump-tree-original" }
> +subroutine example_criticial ()
> +  use omp_lib

Likewise.

So I suppose you'll either have to put these testcases into 'libgomp', or
we'll have to invent something else?  Jakub, is there a reason why for
build-tree testing we can't just add '-I[build-tree]/libgomp' etc. in
'gcc.dg/gomp/gomp.exp' etc.?


Grüße
 Thomas

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Jakub Jelinek July 22, 2020, 9:16 a.m. UTC | #7
On Wed, Jul 22, 2020 at 11:09:06AM +0200, Thomas Schwinge wrote:
> So I suppose you'll either have to put these testcases into 'libgomp', or
> we'll have to invent something else?

Indeed.

> Jakub, is there a reason why for
> build-tree testing we can't just add '-I[build-tree]/libgomp' etc. in
> 'gcc.dg/gomp/gomp.exp' etc.?

I guess historic reasons.  E.g. g++.dg/ adds those and -L for libstdc++ too,
but then most of the C++ tests that test primarily the compiler and
sometimes use headers and even more often the runtime library are there.
On the gomp side, libgomp/testsuite has been used for both compile and
link/runtime tests that need the runtime library and its headers, while
gcc/testsuite/*/gomp/ has been left for tests that don't need any of those.

	Jakub
Tobias Burnus July 22, 2020, 9:25 a.m. UTC | #8
On 7/22/20 11:09 AM, Thomas Schwinge wrote:

> For build-tree testing, that'll pick up the *system* 'omp.h', thus:
>      FAIL: c-c++-common/gomp/critical-hint-1.c (test for excess errors)
>      Excess errors:
>      [...]/c-c++-common/gomp/critical-hint-1.c:10:33: error: 'omp_sync_hint_none' undeclared (first use in this function); did you mean 'omp_lock_hint_none'?
>      [...]

HEADER FILE:
For "ISO_Fortran_binding.h", we use:
   #include "../../../libgfortran/ISO_Fortran_binding.h"
Can you check whether something like that also works for 'omp.h'?
(Additional "../" and "libgomp/".)

Can you check whether that helps? For some reasons, those do
not fail here.

> Similar:
>      FAIL: gfortran.dg/gomp/critical-hint-1.f90   -O  (test for excess errors)
>      Excess errors:
>      [...]/gfortran.dg/gomp/critical-hint-1.f90:2:7: Fatal Error: Cannot open module file 'omp_lib.mod' for reading at (1): No such file or directory

FORTRAN MODULE
Then I have the question why, e.g., "use openacc_kinds" in
gfortran.dg/goacc/acc_on_device-2.f95 does work. What's different there?

The *exp file does not seem to be any different (except that goacc.exp
adds "dg-compile-aux-modules").


Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Jakub Jelinek July 22, 2020, 9:28 a.m. UTC | #9
On Wed, Jul 22, 2020 at 11:25:47AM +0200, Tobias Burnus wrote:
> On 7/22/20 11:09 AM, Thomas Schwinge wrote:
> 
> > For build-tree testing, that'll pick up the *system* 'omp.h', thus:
> >      FAIL: c-c++-common/gomp/critical-hint-1.c (test for excess errors)
> >      Excess errors:
> >      [...]/c-c++-common/gomp/critical-hint-1.c:10:33: error: 'omp_sync_hint_none' undeclared (first use in this function); did you mean 'omp_lock_hint_none'?
> >      [...]
> 
> HEADER FILE:
> For "ISO_Fortran_binding.h", we use:
>   #include "../../../libgfortran/ISO_Fortran_binding.h"
> Can you check whether something like that also works for 'omp.h'?
> (Additional "../" and "libgomp/".)

This can't work, because unlike ISO_Fortran_binding.h, omp.h is
a generated file, so it doesn't appear in the source directory.

	Jakub
Jakub Jelinek July 22, 2020, 9:36 a.m. UTC | #10
On Wed, Jul 22, 2020 at 11:25:47AM +0200, Tobias Burnus wrote:
> FORTRAN MODULE
> Then I have the question why, e.g., "use openacc_kinds" in
> gfortran.dg/goacc/acc_on_device-2.f95 does work. What's different there?

Because that test defines its own module with that name:
module openacc_kinds
  implicit none

  integer, parameter :: acc_device_kind = 4

end module openacc_kinds
Ditto openacc module.

It is like e.g. many g++.dg/ tests which intentionally don't include the
libstdc++ headers, but instead provide minimal definitions of whatever they
need (e.g. placement new, the spaceship stuff, ...).

	Jakub
Tobias Burnus July 22, 2020, 10:15 a.m. UTC | #11
Now moved to libgomp, cf. attachment.

Tobias

On 7/22/20 11:16 AM, Jakub Jelinek wrote:
> On Wed, Jul 22, 2020 at 11:09:06AM +0200, Thomas Schwinge wrote:
>> So I suppose you'll either have to put these testcases into 'libgomp', or
>> we'll have to invent something else?
> Indeed.
>
>> Jakub, is there a reason why for
>> build-tree testing we can't just add '-I[build-tree]/libgomp' etc. in
>> 'gcc.dg/gomp/gomp.exp' etc.?
> I guess historic reasons.  E.g. g++.dg/ adds those and -L for libstdc++ too,
> but then most of the C++ tests that test primarily the compiler and
> sometimes use headers and even more often the runtime library are there.
> On the gomp side, libgomp/testsuite has been used for both compile and
> link/runtime tests that need the runtime library and its headers, while
> gcc/testsuite/*/gomp/ has been left for tests that don't need any of those.
>
>       Jakub
>
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
diff mbox series

Patch

OpenMP: Fixes for omp critical + hint

gcc/c-family/ChangeLog:

	* c-omp.c (c_finish_omp_critical): Check for no name but
	nonzero hint provided.

gcc/c/ChangeLog:

	* c-parser.c (c_parser_omp_clause_hint): Require nonnegative hint clause.
	(c_parser_omp_critical): Permit hint clause without named critical.
	(c_parser_omp_construct): Don't assert if error_mark_node is returned.

gcc/cp/ChangeLog:

	* parser.c (cp_parser_omp_clause_hint): Require nonnegative hint.
	(cp_parser_omp_critical): Permit hint clause without named critical.

gcc/fortran/ChangeLog:

	* openmp.c (gfc_match_omp_critical): Fix handling hints; permit
	hint clause without named critical.
	(resolve_omp_clauses): Require nonnegative constant integer
	for the hint clause.
	(gfc_resolve_omp_directive): Check for no name but
        nonzero value for hint clause.
	* parse.c (parse_omp_structured_block): Fix same-name check
	for critical.
	* trans-openmp.c (gfc_trans_omp_critical): Handle hint clause properly.

libgomp/ChangeLog:

	* omp_lib.f90.in: Add omp_sync_hint_* and omp_sync_hint_kind.
	* omp_lib.h.in: Likewise.

gcc/testsuite/ChangeLog:

	* g++.dg/gomp/critical-3.C: Add nameless critical with hint testcase.
	* c-c++-common/gomp/critical-hint-1.c: New test.
	* c-c++-common/gomp/critical-hint-2.c: New test.
	* gfortran.dg/gomp/critical-hint-1.f90: New test.
	* gfortran.dg/gomp/critical-hint-2.f90: New test.

 gcc/c-family/c-omp.c                               | 12 +++
 gcc/c/c-parser.c                                   | 26 ++----
 gcc/cp/parser.c                                    | 20 +++--
 gcc/fortran/openmp.c                               | 33 +++++---
 gcc/fortran/parse.c                                |  3 +-
 gcc/fortran/trans-openmp.c                         | 18 +++--
 gcc/testsuite/c-c++-common/gomp/critical-hint-1.c  | 47 +++++++++++
 gcc/testsuite/c-c++-common/gomp/critical-hint-2.c  | 36 +++++++++
 gcc/testsuite/g++.dg/gomp/critical-3.C             |  9 +++
 gcc/testsuite/gfortran.dg/gomp/critical-hint-1.f90 | 94 ++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/gomp/critical-hint-2.f90 | 65 +++++++++++++++
 libgomp/omp_lib.f90.in                             | 27 +++++--
 libgomp/omp_lib.h.in                               | 16 +++-
 13 files changed, 357 insertions(+), 49 deletions(-)

diff --git a/gcc/c-family/c-omp.c b/gcc/c-family/c-omp.c
index 1ca2802c64f..9f306554a2b 100644
--- a/gcc/c-family/c-omp.c
+++ b/gcc/c-family/c-omp.c
@@ -106,6 +106,18 @@  c_finish_omp_taskgroup (location_t loc, tree body, tree clauses)
 tree
 c_finish_omp_critical (location_t loc, tree body, tree name, tree clauses)
 {
+  gcc_assert (!clauses || OMP_CLAUSE_CODE (clauses) == OMP_CLAUSE_HINT);
+  if (name == NULL_TREE && clauses != NULL_TREE
+      && INTEGRAL_TYPE_P (TREE_TYPE (OMP_CLAUSE_HINT_EXPR (clauses)))
+      && tree_fits_shwi_p (OMP_CLAUSE_HINT_EXPR (clauses))
+      && tree_to_shwi (OMP_CLAUSE_HINT_EXPR (clauses)) != 0)
+    {
+      error_at (OMP_CLAUSE_LOCATION (clauses),
+		"%<#pragma omp critical%> with %<hint%> clause requires "
+		"a name, except when %<omp_sync_hint_none%> is used");
+      return error_mark_node;
+    }
+
   tree stmt = make_node (OMP_CRITICAL);
   TREE_TYPE (stmt) = void_type_node;
   OMP_CRITICAL_BODY (stmt) = body;
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 275b8a38255..c4f5445c22f 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -13901,16 +13901,15 @@  c_parser_omp_clause_hint (c_parser *parser, tree list)
       expr = convert_lvalue_to_rvalue (expr_loc, expr, false, true);
       tree c, t = expr.value;
       t = c_fully_fold (t, false, NULL);
-
-      parens.skip_until_found_close (parser);
-
       if (!INTEGRAL_TYPE_P (TREE_TYPE (t))
-	  || TREE_CODE (t) != INTEGER_CST)
+	  || TREE_CODE (t) != INTEGER_CST
+	  || tree_to_shwi (t) < 0)
 	{
-	  c_parser_error (parser, "expected constant integer expression");
+	  c_parser_error (parser, "expected constant integer expression "
+				  "with valid sync-hint value");
 	  return list;
 	}
-
+      parens.skip_until_found_close (parser);
       check_no_duplicate_clause (list, OMP_CLAUSE_HINT, "hint");
 
       c = build_omp_clause (hint_loc, OMP_CLAUSE_HINT);
@@ -17795,18 +17794,9 @@  c_parser_omp_critical (location_t loc, c_parser *parser, bool *if_p)
       if (c_parser_next_token_is (parser, CPP_COMMA)
 	  && c_parser_peek_2nd_token (parser)->type == CPP_NAME)
 	c_parser_consume_token (parser);
-
-      clauses = c_parser_omp_all_clauses (parser,
-					  OMP_CRITICAL_CLAUSE_MASK,
-					  "#pragma omp critical");
     }
-  else
-    {
-      if (c_parser_next_token_is_not (parser, CPP_PRAGMA_EOL))
-	c_parser_error (parser, "expected %<(%> or end of line");
-      c_parser_skip_to_pragma_eol (parser);
-    }
-
+  clauses = c_parser_omp_all_clauses (parser, OMP_CRITICAL_CLAUSE_MASK,
+				      "#pragma omp critical");
   stmt = c_parser_omp_structured_block (parser, if_p);
   return c_finish_omp_critical (loc, stmt, name, clauses);
 }
@@ -21537,7 +21527,7 @@  c_parser_omp_construct (c_parser *parser, bool *if_p)
       gcc_unreachable ();
     }
 
-  if (stmt)
+  if (stmt && stmt != error_mark_node)
     gcc_assert (EXPR_LOCATION (stmt) != UNKNOWN_LOCATION);
 }
 
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 11db02418bc..1edc649e075 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -35382,12 +35382,21 @@  cp_parser_omp_clause_hint (cp_parser *parser, tree list, location_t location)
 
   t = cp_parser_assignment_expression (parser);
 
+  if (t != error_mark_node)
+    {
+      t = fold_non_dependent_expr (t);
+      if (!value_dependent_expression_p (t)
+	  && (!INTEGRAL_TYPE_P (TREE_TYPE (t))
+	      || !tree_fits_shwi_p (t)
+	      || tree_to_shwi (t) < 0))
+	error_at (location, "expected constant integer expression with "
+			    "valid sync-hint value");
+    }
   if (t == error_mark_node
       || !parens.require_close (parser))
     cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true,
 					   /*or_comma=*/false,
 					   /*consume_paren=*/true);
-
   check_no_duplicate_clause (list, OMP_CLAUSE_HINT, "hint", location);
 
   c = build_omp_clause (location, OMP_CLAUSE_HINT);
@@ -38209,13 +38218,10 @@  cp_parser_omp_critical (cp_parser *parser, cp_token *pragma_tok, bool *if_p)
       if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA)
 	  && cp_lexer_nth_token_is (parser->lexer, 2, CPP_NAME))
 	cp_lexer_consume_token (parser->lexer);
-
-      clauses = cp_parser_omp_all_clauses (parser,
-					   OMP_CRITICAL_CLAUSE_MASK,
-					   "#pragma omp critical", pragma_tok);
     }
-  else
-    cp_parser_require_pragma_eol (parser, pragma_tok);
+
+  clauses = cp_parser_omp_all_clauses (parser, OMP_CRITICAL_CLAUSE_MASK,
+				       "#pragma omp critical", pragma_tok);
 
   stmt = cp_parser_omp_structured_block (parser, if_p);
   return c_finish_omp_critical (input_location, stmt, name, clauses);
diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 7de2f6e1b1d..58552af0982 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -2631,15 +2631,10 @@  gfc_match_omp_critical (void)
   gfc_omp_clauses *c = NULL;
 
   if (gfc_match (" ( %n )", n) != MATCH_YES)
-    {
-      n[0] = '\0';
-      if (gfc_match_omp_eos () != MATCH_YES)
-	{
-	  gfc_error ("Unexpected junk after $OMP CRITICAL statement at %C");
-	  return MATCH_ERROR;
-	}
-    }
-  else if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_HINT)) != MATCH_YES)
+    n[0] = '\0';
+
+  if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_HINT),
+			     /* first = */ n[0] == '\0') != MATCH_YES)
     return MATCH_ERROR;
 
   new_st.op = EXEC_OMP_CRITICAL;
@@ -5000,7 +4995,14 @@  resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
   if (omp_clauses->device)
     resolve_nonnegative_int_expr (omp_clauses->device, "DEVICE");
   if (omp_clauses->hint)
-    resolve_scalar_int_expr (omp_clauses->hint, "HINT");
+    {
+      resolve_scalar_int_expr (omp_clauses->hint, "HINT");
+    if (omp_clauses->hint->ts.type != BT_INTEGER
+	|| omp_clauses->hint->expr_type != EXPR_CONSTANT
+	|| mpz_sgn (omp_clauses->hint->value.integer) < 0)
+      gfc_error ("Value of HINT clause at %L shall be a valid "
+		 "constant hint expression", &omp_clauses->hint->where);
+    }
   if (omp_clauses->priority)
     resolve_nonnegative_int_expr (omp_clauses->priority, "PRIORITY");
   if (omp_clauses->dist_chunk_size)
@@ -6515,6 +6517,17 @@  gfc_resolve_omp_directive (gfc_code *code, gfc_namespace *ns ATTRIBUTE_UNUSED)
     case EXEC_OMP_ATOMIC:
       resolve_omp_atomic (code);
       break;
+    case EXEC_OMP_CRITICAL:
+      if (code->ext.omp_clauses)
+	resolve_omp_clauses (code, code->ext.omp_clauses, NULL);
+      if (!code->ext.omp_clauses->critical_name
+	  && code->ext.omp_clauses->hint
+	  && code->ext.omp_clauses->hint->ts.type == BT_INTEGER
+	  && code->ext.omp_clauses->hint->expr_type == EXPR_CONSTANT
+	  && mpz_sgn (code->ext.omp_clauses->hint->value.integer) != 0)
+	gfc_error ("OMP CRITICAL at %L with HINT clause requires a NAME, "
+		   "except when omp_sync_hint_none is used", &code->loc);
+      break;
     default:
       break;
     }
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index d30208febb1..96fd4aaee5e 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -5384,7 +5384,8 @@  parse_omp_structured_block (gfc_statement omp_st, bool workshare_stmts_only)
       cp->ext.omp_clauses->nowait |= new_st.ext.omp_bool;
       break;
     case EXEC_OMP_END_CRITICAL:
-      if (((cp->ext.omp_clauses == NULL) ^ (new_st.ext.omp_name == NULL))
+      if (((cp->ext.omp_clauses->critical_name == NULL)
+	    ^ (new_st.ext.omp_name == NULL))
 	  || (new_st.ext.omp_name != NULL
 	      && strcmp (cp->ext.omp_clauses->critical_name,
 			 new_st.ext.omp_name) != 0))
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
index 80929c77cc6..a63000be314 100644
--- a/gcc/fortran/trans-openmp.c
+++ b/gcc/fortran/trans-openmp.c
@@ -4242,12 +4242,20 @@  gfc_trans_omp_cancellation_point (gfc_code *code)
 static tree
 gfc_trans_omp_critical (gfc_code *code)
 {
-  tree name = NULL_TREE, stmt;
-  if (code->ext.omp_clauses != NULL)
+  stmtblock_t block;
+  tree stmt, name = NULL_TREE;
+  if (code->ext.omp_clauses->critical_name != NULL)
     name = get_identifier (code->ext.omp_clauses->critical_name);
-  stmt = gfc_trans_code (code->block->next);
-  return build3_loc (input_location, OMP_CRITICAL, void_type_node, stmt,
-		     NULL_TREE, name);
+  gfc_start_block (&block);
+  stmt = make_node (OMP_CRITICAL);
+  TREE_TYPE (stmt) = void_type_node;
+  OMP_CRITICAL_BODY (stmt) = gfc_trans_code (code->block->next);
+  OMP_CRITICAL_NAME (stmt) = name;
+  OMP_CRITICAL_CLAUSES (stmt) = gfc_trans_omp_clauses (&block,
+						       code->ext.omp_clauses,
+						       code->loc);
+  gfc_add_expr_to_block (&block, stmt);
+  return gfc_finish_block (&block);
 }
 
 typedef struct dovar_init_d {
diff --git a/gcc/testsuite/c-c++-common/gomp/critical-hint-1.c b/gcc/testsuite/c-c++-common/gomp/critical-hint-1.c
new file mode 100644
index 00000000000..510f8abef80
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/critical-hint-1.c
@@ -0,0 +1,47 @@ 
+#include <omp.h>
+
+void
+example_criticial ()
+{
+  int a, b;
+  #pragma omp parallel for
+  for (int i = 0; i < 10; ++i)
+    {
+      #pragma omp critical hint(omp_sync_hint_none)  /* OK */
+      a += i;
+      #pragma omp critical (HASH) hint(omp_sync_hint_none)  /* OK */
+      a += i;
+      #pragma omp critical (HASH2) hint(omp_sync_hint_uncontended)  /* OK */
+      a += i;
+      #pragma omp critical (HASH3) hint(omp_sync_hint_contended)  /* OK */
+      a += i;
+      #pragma omp critical (HASH4) hint(omp_sync_hint_speculative)  /* OK */
+      a += i;
+      #pragma omp critical (HASH5) hint(omp_sync_hint_nonspeculative)  /* OK */
+      a += i;
+      #pragma omp critical (HASH6) hint(omp_sync_hint_contended + omp_sync_hint_speculative)  /* OK */
+      a += i;
+      #pragma omp critical (HASH6) hint(omp_sync_hint_contended | omp_sync_hint_speculative)  /* OK */
+      a += i;
+
+      /* Accepted but invalid: different hint for same name. */
+      #pragma omp critical (HASH6) hint(omp_sync_hint_uncontended + omp_sync_hint_speculative)  
+      a += i;
+      /* Accepted but invalid: Some random integer expr. */
+      #pragma omp critical (HASH) hint(omp_sync_hint_speculative + 1 + 2)
+      a += i;
+
+      #pragma omp critical (HASH) hint(-3)  /* { dg-error "expected constant integer expression" } */
+      a += i;
+      #pragma omp critical (HASH2) hint(b)  /* { dg-error "constant integer expression" } */
+      a += i;
+/*
+  Fails with gcc as 'expected identifier' and
+        with g++ as "clause requires a name, except when 'omp_sync_hint_none'"
+      #pragma omp critical () hint(omp_sync_hint_speculative)
+      a += i;
+*/
+      #pragma omp critical hint(omp_sync_hint_speculative)  /* { dg-error "with 'hint' clause requires a name, except when 'omp_sync_hint_none' is used" } */
+      a += i;
+    }
+}
diff --git a/gcc/testsuite/c-c++-common/gomp/critical-hint-2.c b/gcc/testsuite/c-c++-common/gomp/critical-hint-2.c
new file mode 100644
index 00000000000..effe24a63ec
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/critical-hint-2.c
@@ -0,0 +1,36 @@ 
+/* { dg-additional-options "-fdump-tree-original" } */
+#include <omp.h>
+
+void
+example_criticial ()
+{
+  int a, b;
+  #pragma omp parallel for
+  for (int i = 0; i < 10; ++i)
+    {
+      #pragma omp critical hint(omp_sync_hint_none)
+      a += i;
+      #pragma omp critical (HASH1) hint(omp_sync_hint_none)
+      a += i;
+      #pragma omp critical (HASH2) hint(omp_sync_hint_uncontended)
+      a += i;
+      #pragma omp critical (HASH3) hint(omp_sync_hint_contended)
+      a += i;
+      #pragma omp critical (HASH4) hint(omp_sync_hint_speculative)
+      a += i;
+      #pragma omp critical (HASH5) hint(omp_sync_hint_nonspeculative)
+      a += i;
+      #pragma omp critical (HASH6) hint(omp_sync_hint_contended + omp_sync_hint_speculative)
+      a += i;
+      #pragma omp critical (HASH7) hint(omp_sync_hint_contended | omp_sync_hint_speculative)
+      a += i;
+    }
+}
+
+/* { dg-final { scan-tree-dump-times "omp critical \\(HASH1\\) hint\\(0\\)" 1 "original" } } */
+/* { dg-final { scan-tree-dump-times "omp critical \\(HASH2\\) hint\\(1\\)" 1 "original" } } */
+/* { dg-final { scan-tree-dump-times "omp critical \\(HASH3\\) hint\\(2\\)" 1 "original" } } */
+/* { dg-final { scan-tree-dump-times "omp critical \\(HASH4\\) hint\\(8\\)" 1 "original" } } */
+/* { dg-final { scan-tree-dump-times "omp critical \\(HASH5\\) hint\\(4\\)" 1 "original" } } */
+/* { dg-final { scan-tree-dump-times "omp critical \\(HASH6\\) hint\\(10\\)" 1 "original" } } */
+/* { dg-final { scan-tree-dump-times "omp critical \\(HASH7\\) hint\\(10\\)" 1 "original" } } */
diff --git a/gcc/testsuite/g++.dg/gomp/critical-3.C b/gcc/testsuite/g++.dg/gomp/critical-3.C
index b8dc496198f..25e486b269e 100644
--- a/gcc/testsuite/g++.dg/gomp/critical-3.C
+++ b/gcc/testsuite/g++.dg/gomp/critical-3.C
@@ -8,6 +8,14 @@  foo (void)
   i++;
 }
 
+template <int N>
+void
+foobar (void)
+{
+  #pragma omp critical hint (N + 0)
+  i++;
+}
+
 template <int N>
 void
 bar (void)
@@ -28,6 +36,7 @@  void
 test ()
 {
   foo <0> ();
+  foobar <0> ();
   bar <0> ();
   baz (0.0);
 }
diff --git a/gcc/testsuite/gfortran.dg/gomp/critical-hint-1.f90 b/gcc/testsuite/gfortran.dg/gomp/critical-hint-1.f90
new file mode 100644
index 00000000000..c26b617f1bd
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/critical-hint-1.f90
@@ -0,0 +1,94 @@ 
+subroutine example_criticial ()
+  use omp_lib
+  implicit none
+  integer, parameter :: my_omp_hint = omp_sync_hint_contended
+  integer i, a, b
+
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH0) hint(my_omp_hint)  ! OK
+      a = a + i;
+      !$omp end critical (HASH0)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH1) hint(omp_sync_hint_none)  ! OK
+      a = a + i;
+      !$omp end critical (HASH1)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH2) hint(omp_sync_hint_uncontended)  ! OK
+      a = a + i;
+      !$omp end critical (HASH2)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH3) hint(omp_sync_hint_contended)  ! OK
+      a = a + i;
+      !$omp end critical (HASH3)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH4) hint(omp_sync_hint_speculative)  ! OK
+      a = a + i;
+      !$omp end critical (HASH4)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH5) hint(omp_sync_hint_nonspeculative)  ! OK
+      a = a + i;
+      !$omp end critical (HASH5)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH6) hint(omp_sync_hint_contended + omp_sync_hint_speculative)  ! OK
+      a = a + i;
+      !$omp end critical (HASH6)
+  end do
+
+  !$omp parallel do
+  do i = 1, 10
+      ! Accepted but invalid: different hint for same name.
+      !$omp critical (HASH6) hint(omp_sync_hint_contended + omp_sync_hint_speculative)  ! OK
+      a = a + i;
+      !$omp end critical (HASH6)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      ! Accepted but invalid: Some random integer expr.
+      !$omp critical (HASH) hint(1 + 2)
+      a = a + i;
+      !$omp end critical (HASH)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH) hint(-3)  ! { dg-error "shall be a valid constant hint expression" }
+      a = a + i;
+      !$omp end critical (HASH)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH2) hint(b)  ! { dg-error "shall be a valid constant hint expression" }
+      a = a + i;
+      !$omp end critical (HASH2)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical () hint(omp_hint_speculative)  ! { dg-error "Invalid character in name" }
+      a = a + i;
+!      !$omp end critical
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical hint(omp_sync_hint_none)  ! OK
+      a = a + i;
+      !$omp end critical
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical hint(omp_sync_hint_contended)  ! { dg-error "CRITICAL at .1. with HINT clause requires a NAME, except when omp_sync_hint_none is used" }
+      a = a + i;
+      !$omp end critical
+  end do
+end
diff --git a/gcc/testsuite/gfortran.dg/gomp/critical-hint-2.f90 b/gcc/testsuite/gfortran.dg/gomp/critical-hint-2.f90
new file mode 100644
index 00000000000..15d6206a438
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/critical-hint-2.f90
@@ -0,0 +1,65 @@ 
+! { dg-additional-options "-fdump-tree-original" }
+subroutine example_criticial ()
+  use omp_lib
+  implicit none
+  integer, parameter :: my_omp_hint = omp_sync_hint_contended
+  integer i, a, b
+
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH0) hint(my_omp_hint)
+      a = a + i;
+      !$omp end critical (HASH0)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH1), hint(omp_sync_hint_none)
+      a = a + i;
+      !$omp end critical (HASH1)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH2) hint(omp_sync_hint_uncontended)
+      a = a + i;
+      !$omp end critical (HASH2)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH3) hint(omp_sync_hint_contended)
+      a = a + i;
+      !$omp end critical (HASH3)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH4) hint(omp_sync_hint_speculative)
+      a = a + i;
+      !$omp end critical (HASH4)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH5) hint(omp_sync_hint_nonspeculative)
+      a = a + i;
+      !$omp end critical (HASH5)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical (HASH6), hint(omp_sync_hint_contended + omp_sync_hint_speculative)
+      a = a + i;
+      !$omp end critical (HASH6)
+  end do
+  !$omp parallel do
+  do i = 1, 10
+      !$omp critical hint(omp_sync_hint_none + omp_sync_hint_none)
+      a = a + i;
+      !$omp end critical
+  end do
+end
+
+! { dg-final { scan-tree-dump-times "omp critical \\(hash0\\) hint\\(2\\)" 1 "original" } }
+! { dg-final { scan-tree-dump-times "omp critical \\(hash1\\) hint\\(0\\)" 1 "original" } }
+! { dg-final { scan-tree-dump-times "omp critical \\(hash2\\) hint\\(1\\)" 1 "original" } }
+! { dg-final { scan-tree-dump-times "omp critical \\(hash3\\) hint\\(2\\)" 1 "original" } }
+! { dg-final { scan-tree-dump-times "omp critical \\(hash4\\) hint\\(8\\)" 1 "original" } }
+! { dg-final { scan-tree-dump-times "omp critical \\(hash5\\) hint\\(4\\)" 1 "original" } }
+! { dg-final { scan-tree-dump-times "omp critical \\(hash6\\) hint\\(10\\)" 1 "original" } }
+! { dg-final { scan-tree-dump-times "omp critical hint\\(0\\)" 1 "original" } }
diff --git a/libgomp/omp_lib.f90.in b/libgomp/omp_lib.f90.in
index 666b5152a5f..b22bcbaf770 100644
--- a/libgomp/omp_lib.f90.in
+++ b/libgomp/omp_lib.f90.in
@@ -31,7 +31,8 @@ 
         integer, parameter :: omp_nest_lock_kind = @OMP_NEST_LOCK_KIND@
         integer, parameter :: omp_sched_kind = 4
         integer, parameter :: omp_proc_bind_kind = 4
-        integer, parameter :: omp_lock_hint_kind = 4
+        integer, parameter :: omp_sync_hint_kind = 4
+        integer, parameter :: omp_lock_hint_kind = omp_sync_hint_kind
         integer, parameter :: omp_pause_resource_kind = 4
         integer, parameter :: omp_allocator_handle_kind = c_intptr_t
         integer, parameter :: omp_alloctrait_key_kind = c_int
@@ -52,15 +53,29 @@ 
         integer (omp_proc_bind_kind), &
                  parameter :: omp_proc_bind_spread = 4
         integer (omp_lock_hint_kind), &
-                 parameter :: omp_lock_hint_none = 0
+                 parameter :: omp_sync_hint_none = 0
         integer (omp_lock_hint_kind), &
-                 parameter :: omp_lock_hint_uncontended = 1
+                 parameter :: omp_lock_hint_none = omp_sync_hint_none
         integer (omp_lock_hint_kind), &
-                 parameter :: omp_lock_hint_contended = 2
+                 parameter :: omp_sync_hint_uncontended = 1
         integer (omp_lock_hint_kind), &
-                 parameter :: omp_lock_hint_nonspeculative = 4
+                 parameter :: omp_lock_hint_uncontended &
+                 = omp_sync_hint_uncontended
         integer (omp_lock_hint_kind), &
-                 parameter :: omp_lock_hint_speculative = 8
+                 parameter :: omp_sync_hint_contended = 2
+        integer (omp_lock_hint_kind), &
+                 parameter :: omp_lock_hint_contended &
+                 = omp_sync_hint_contended
+        integer (omp_lock_hint_kind), &
+                 parameter :: omp_sync_hint_nonspeculative = 4
+        integer (omp_lock_hint_kind), &
+                 parameter :: omp_lock_hint_nonspeculative &
+                 = omp_sync_hint_nonspeculative
+        integer (omp_lock_hint_kind), &
+                 parameter :: omp_sync_hint_speculative = 8
+        integer (omp_lock_hint_kind), &
+                 parameter :: omp_lock_hint_speculative &
+                 = omp_sync_hint_speculative
         integer (kind=omp_pause_resource_kind), &
                  parameter :: omp_pause_soft = 1
         integer (kind=omp_pause_resource_kind), &
diff --git a/libgomp/omp_lib.h.in b/libgomp/omp_lib.h.in
index 34babe93ab9..c7d444d4a97 100644
--- a/libgomp/omp_lib.h.in
+++ b/libgomp/omp_lib.h.in
@@ -46,17 +46,29 @@ 
       parameter (omp_proc_bind_master = 2)
       parameter (omp_proc_bind_close = 3)
       parameter (omp_proc_bind_spread = 4)
+      integer omp_sync_hint_kind
       integer omp_lock_hint_kind
-      parameter (omp_lock_hint_kind = 4)
+      parameter (omp_sync_hint_kind = 4)
+      parameter (omp_lock_hint_kind = omp_sync_hint_kind)
+      integer (omp_sync_hint_kind) omp_sync_hint_none
       integer (omp_lock_hint_kind) omp_lock_hint_none
+      integer (omp_sync_hint_kind) omp_sync_hint_uncontended
       integer (omp_lock_hint_kind) omp_lock_hint_uncontended
-      integer (omp_lock_hint_kind) omp_lock_hint_contended
+      integer (omp_sync_hint_kind) omp_sync_hint_contended
+      integer (omp_sync_hint_kind) omp_lock_hint_contended
+      integer (omp_lock_hint_kind) omp_sync_hint_nonspeculative
       integer (omp_lock_hint_kind) omp_lock_hint_nonspeculative
+      integer (omp_sync_hint_kind) omp_sync_hint_speculative
       integer (omp_lock_hint_kind) omp_lock_hint_speculative
+      parameter (omp_sync_hint_none = 0)
       parameter (omp_lock_hint_none = 0)
+      parameter (omp_sync_hint_uncontended = 1)
       parameter (omp_lock_hint_uncontended = 1)
+      parameter (omp_sync_hint_contended = 2)
       parameter (omp_lock_hint_contended = 2)
+      parameter (omp_sync_hint_nonspeculative = 4)
       parameter (omp_lock_hint_nonspeculative = 4)
+      parameter (omp_sync_hint_speculative = 8)
       parameter (omp_lock_hint_speculative = 8)
       parameter (openmp_version = 201511)
       integer omp_pause_resource_kind