[PR81844] Fix condition folding in c_parser_omp_for_loop

Hi,

this patch fixes the wrong-code PR81844, where an omp for loop is 
incorrectly removed by the compiler.


Consider the test-case from the patch.

It contains a omp for condition 'i > 0x7fffffffffffffffULL', where i is 
of type unsigned long long int.


In c_parser_omp_for_loop, we first have:
...
(gdb) call debug_generic_expr (cond)
<<< Unknown tree: c_maybe_const_expr

   i >>> > 9223372036854775807
...

Then we execute:
...
15030		  cond = c_fully_fold (cond, false, NULL);
...

and we have:
...
(gdb) call debug_generic_expr (cond)
(signed long) i < 0
...

Note that the type of the comparison changed from unsigned to signed.

Subsequently, in c_finish_omp_for we do:
...
               /* 2.5.1.  The comparison in the condition is computed in
                  the type of DECL, otherwise the behavior is undefined.

                  For example:
                  long n; int i;
                  i < n;

                  according to ISO will be evaluated as:
                  (long)i < n;

                  We want to force:
                  i < (int)n;  */
               if (TREE_CODE (op0) == NOP_EXPR
                   && decl == TREE_OPERAND (op0, 0))
                 {
                   TREE_OPERAND (cond, 0) = TREE_OPERAND (op0, 0);
                   TREE_OPERAND (cond, 1)
                     = fold_build1_loc (elocus, NOP_EXPR,
                                        TREE_TYPE (decl),
                                    TREE_OPERAND (cond, 1));
                 }
...

and we end up with:
...
(gdb) call debug_generic_expr (cond)
i < 0
...
which is always false.


AFAIU, the problem is that the optimization in c_finish_omp_for assumes 
that the operand type of the condition is the same as in the source 
code, while c_fully_fold breaks that assumption.


The patch fixes this by folding only the operands of the condition.

Bootstrapped and reg-tested on x86_64.

OK for trunk?

Thanks,
- Tom

On Mon, Aug 14, 2017 at 10:25:22AM +0200, Tom de Vries wrote:
> 2017-08-14  Tom de Vries  <tom@codesourcery.com>
> 
> 	PR c/81844

Please use PR c/81875 instead, now that you've filed it.

> 	* c-parser.c (c_parser_omp_for_loop): Fix condition folding.

Fold only operands of cond, not cond itself.
?

> 	* testsuite/libgomp.c/pr81805.c: New test.

Wouldn't it be worth to test it also for C++?  I know we don't have
libgomp.c-c++-common (maybe we should add that), so the current
way would be add libgomp.c++/pr81805.C that #includes the other test
source (if you tweak it for C++, it would need #ifdef __cplusplus "C" #endif
for abort).

> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -15027,7 +15027,24 @@ c_parser_omp_for_loop (location_t loc, c_parser *parser, enum tree_code code,
>  
>  	  cond = cond_expr.value;
>  	  cond = c_objc_common_truthvalue_conversion (cond_loc, cond);
> -	  cond = c_fully_fold (cond, false, NULL);
> +	  switch (TREE_CODE (cond))

Just do if (COMPARISON_CLASS_P (cond)) instead of the switch?

> +	    {
> +	    case GT_EXPR:
> +	    case GE_EXPR:
> +	    case LT_EXPR:
> +	    case LE_EXPR:
> +	    case NE_EXPR:
> +	      {
> +		tree op0 = TREE_OPERAND (cond, 0), op1 = TREE_OPERAND (cond, 1);
> +		op0 = c_fully_fold (op0, false, NULL);
> +		op1 = c_fully_fold (op1, false, NULL);
> +		TREE_OPERAND (cond, 0) = op0;
> +		TREE_OPERAND (cond, 1) = op1;
> +	      }
> +	      break;
> +	    default:
> +	      break;
> +	    }
>  	  switch (cond_expr.original_code)
>  	    {
>  	    case GT_EXPR:

Ok with those changes and sorry for the review delay.

	Jakub

> I know we don't have
> libgomp.c-c++-common (maybe we should add that)

Like so?

Ran:
- make check-target-libgomp RUNTESTFLAGS=c.exp=cancel-taskgroup-1.c
- make check-target-libgomp RUNTESTFLAGS=c++.exp=cancel-taskgroup-1.c

Currently running make check-target-libgomp.

OK for trunk if tests pass?

Thanks,
- Tom
Introduce libgomp/testsuite/libgomp.c-c++-common

2017-09-14  Tom de Vries  <tom@codesourcery.com>

	* testsuite/libgomp.c++/cancel-taskgroup-1.C: Remove.
	* testsuite/libgomp.c/cancel-taskgroup-1.c: Move to ...
	* testsuite/libgomp.c-c++-common/cancel-taskgroup-1.c: ... here.
	* testsuite/libgomp.c/c.exp: Include test-cases from
	libgomp.c-c++-common.
	* testsuite/libgomp.c++/c++.exp: Same.  Force c++-mode compilation of .c
	files.

---
 libgomp/testsuite/libgomp.c++/c++.exp              |  9 ++-
 libgomp/testsuite/libgomp.c++/cancel-taskgroup-1.C |  4 --
 .../libgomp.c-c++-common/cancel-taskgroup-1.c      | 70 ++++++++++++++++++++++
 libgomp/testsuite/libgomp.c/c.exp                  |  4 +-
 libgomp/testsuite/libgomp.c/cancel-taskgroup-1.c   | 70 ----------------------
 5 files changed, 81 insertions(+), 76 deletions(-)

diff --git a/libgomp/testsuite/libgomp.c++/c++.exp b/libgomp/testsuite/libgomp.c++/c++.exp
index 0454f95..146b2ba 100644
--- a/libgomp/testsuite/libgomp.c++/c++.exp
+++ b/libgomp/testsuite/libgomp.c++/c++.exp
@@ -22,6 +22,11 @@ dg-init
 # Turn on OpenMP.
 lappend ALWAYS_CFLAGS "additional_flags=-fopenmp"
 
+# Switch into C++ mode.  Otherwise, the libgomp.c-c++-common/*.c
+# files would be compiled as C files.
+set SAVE_GCC_UNDER_TEST "$GCC_UNDER_TEST"
+set GCC_UNDER_TEST "$GCC_UNDER_TEST -x c++"
+
 set blddir [lookfor_file [get_multilibs] libgomp]
 
 
@@ -47,7 +52,9 @@ if { $blddir != "" } {
 
 if { $lang_test_file_found } {
     # Gather a list of all tests.
-    set tests [lsort [find $srcdir/$subdir *.C]]
+    set tests [lsort [concat \
+			  [find $srcdir/$subdir *.C] \
+			  [find $srcdir/$subdir/../libgomp.c-c++-common *.c]]]
 
     if { $blddir != "" } {
         set ld_library_path "$always_ld_library_path:${blddir}/${lang_library_path}"
diff --git a/libgomp/testsuite/libgomp.c++/cancel-taskgroup-1.C b/libgomp/testsuite/libgomp.c++/cancel-taskgroup-1.C
deleted file mode 100644
index 4f66859..0000000
--- a/libgomp/testsuite/libgomp.c++/cancel-taskgroup-1.C
+++ /dev/null
@@ -1,4 +0,0 @@
-// { dg-do run }
-// { dg-set-target-env-var OMP_CANCELLATION "true" }
-
-#include "../libgomp.c/cancel-taskgroup-1.c"
diff --git a/libgomp/testsuite/libgomp.c-c++-common/cancel-taskgroup-1.c b/libgomp/testsuite/libgomp.c-c++-common/cancel-taskgroup-1.c
new file mode 100644
index 0000000..5a80811
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c-c++-common/cancel-taskgroup-1.c
@@ -0,0 +1,70 @@
+/* { dg-do run } */
+/* { dg-set-target-env-var OMP_CANCELLATION "true" } */
+
+#include <stdlib.h>
+#include <omp.h>
+
+struct T { struct T *children[2]; int val; };
+
+struct T *
+search (struct T *tree, int val, int lvl)
+{
+  if (tree == NULL || tree->val == val)
+    return tree;
+  struct T *ret = NULL;
+  int i;
+  for (i = 0; i < 2; i++)
+    #pragma omp task shared(ret) if(lvl < 10)
+    {
+      struct T *r = search (tree->children[i], val, lvl + 1);
+      if (r)
+	{
+	  #pragma omp atomic write
+	  ret = r;
+	  #pragma omp cancel taskgroup
+	}
+    }
+  #pragma omp taskwait
+  return ret;
+}
+
+struct T *
+searchp (struct T *tree, int val)
+{
+  struct T *ret;
+  #pragma omp parallel shared(ret) firstprivate (tree, val)
+  #pragma omp single
+  #pragma omp taskgroup
+  ret = search (tree, val, 0);
+  return ret;
+}
+
+int
+main ()
+{
+  /* Must be power of two minus 1.  */
+  int size = 0x7ffff;
+  struct T *trees = (struct T *) malloc (size * sizeof (struct T));
+  if (trees == NULL)
+    return 0;
+  int i, l = 1, b = 0;
+  for (i = 0; i < size; i++)
+    {
+      if (i == l)
+	{
+	  b = l;
+	  l = l * 2 + 1;
+	}
+      trees[i].val = i;
+      trees[i].children[0] = l == size ? NULL : &trees[l + (i - b) * 2];
+      trees[i].children[1] = l == size ? NULL : &trees[l + (i - b) * 2 + 1];
+    }
+  for (i = 0; i < 50; i++)
+    {
+      int v = random () & size;
+      if (searchp (&trees[0], v) != &trees[v])
+	abort ();
+    }
+  free (trees);
+  return 0;
+}
diff --git a/libgomp/testsuite/libgomp.c/c.exp b/libgomp/testsuite/libgomp.c/c.exp
index 300b921..31bdd57 100644
--- a/libgomp/testsuite/libgomp.c/c.exp
+++ b/libgomp/testsuite/libgomp.c/c.exp
@@ -24,7 +24,9 @@ dg-init
 lappend ALWAYS_CFLAGS "additional_flags=-fopenmp"
 
 # Gather a list of all tests.
-set tests [lsort [find $srcdir/$subdir *.c]]
+set tests [lsort [concat \
+		      [find $srcdir/$subdir *.c] \
+		      [find $srcdir/$subdir/../libgomp.c-c++-common *.c]]]
 
 set ld_library_path $always_ld_library_path
 append ld_library_path [gcc-set-multilib-library-path $GCC_UNDER_TEST]
diff --git a/libgomp/testsuite/libgomp.c/cancel-taskgroup-1.c b/libgomp/testsuite/libgomp.c/cancel-taskgroup-1.c
deleted file mode 100644
index 5a80811..0000000
--- a/libgomp/testsuite/libgomp.c/cancel-taskgroup-1.c
+++ /dev/null
@@ -1,70 +0,0 @@
-/* { dg-do run } */
-/* { dg-set-target-env-var OMP_CANCELLATION "true" } */
-
-#include <stdlib.h>
-#include <omp.h>
-
-struct T { struct T *children[2]; int val; };
-
-struct T *
-search (struct T *tree, int val, int lvl)
-{
-  if (tree == NULL || tree->val == val)
-    return tree;
-  struct T *ret = NULL;
-  int i;
-  for (i = 0; i < 2; i++)
-    #pragma omp task shared(ret) if(lvl < 10)
-    {
-      struct T *r = search (tree->children[i], val, lvl + 1);
-      if (r)
-	{
-	  #pragma omp atomic write
-	  ret = r;
-	  #pragma omp cancel taskgroup
-	}
-    }
-  #pragma omp taskwait
-  return ret;
-}
-
-struct T *
-searchp (struct T *tree, int val)
-{
-  struct T *ret;
-  #pragma omp parallel shared(ret) firstprivate (tree, val)
-  #pragma omp single
-  #pragma omp taskgroup
-  ret = search (tree, val, 0);
-  return ret;
-}
-
-int
-main ()
-{
-  /* Must be power of two minus 1.  */
-  int size = 0x7ffff;
-  struct T *trees = (struct T *) malloc (size * sizeof (struct T));
-  if (trees == NULL)
-    return 0;
-  int i, l = 1, b = 0;
-  for (i = 0; i < size; i++)
-    {
-      if (i == l)
-	{
-	  b = l;
-	  l = l * 2 + 1;
-	}
-      trees[i].val = i;
-      trees[i].children[0] = l == size ? NULL : &trees[l + (i - b) * 2];
-      trees[i].children[1] = l == size ? NULL : &trees[l + (i - b) * 2 + 1];
-    }
-  for (i = 0; i < 50; i++)
-    {
-      int v = random () & size;
-      if (searchp (&trees[0], v) != &trees[v])
-	abort ();
-    }
-  free (trees);
-  return 0;
-}

On Thu, Sep 14, 2017 at 07:34:14PM +0000, de Vries, Tom wrote:

> --- a/libgomp/testsuite/libgomp.c++/c++.exp
> +++ b/libgomp/testsuite/libgomp.c++/c++.exp
> @@ -22,6 +22,11 @@ dg-init
>  # Turn on OpenMP.
>  lappend ALWAYS_CFLAGS "additional_flags=-fopenmp"
>  
> +# Switch into C++ mode.  Otherwise, the libgomp.c-c++-common/*.c
> +# files would be compiled as C files.
> +set SAVE_GCC_UNDER_TEST "$GCC_UNDER_TEST"
> +set GCC_UNDER_TEST "$GCC_UNDER_TEST -x c++"
> +
>  set blddir [lookfor_file [get_multilibs] libgomp]
>  
>  
> @@ -47,7 +52,9 @@ if { $blddir != "" } {
>  
>  if { $lang_test_file_found } {
>      # Gather a list of all tests.
> -    set tests [lsort [find $srcdir/$subdir *.C]]
> +    set tests [lsort [concat \
> +			  [find $srcdir/$subdir *.C] \
> +			  [find $srcdir/$subdir/../libgomp.c-c++-common *.c]]]
>  
>      if { $blddir != "" } {
>          set ld_library_path "$always_ld_library_path:${blddir}/${lang_library_path}"

I don't see SAVE_GCC_UNDER_TEST being used anywhere after it is set.
Did you mean to set GCC_UNDER_TEST back to SAVE_GCC_UNDER_TEST at the end of
c++.exp?
libgomp.oacc-c++/c++.exp has:
# See above.
set GCC_UNDER_TEST "$SAVE_GCC_UNDER_TEST"

Otherwise LGTM, thanks.

	Jakub

On 09/14/2017 12:55 PM, Jakub Jelinek wrote:
> Ok with those changes

Hi,

this PR has been fixed on trunk ( 
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=252873 ).

We cannot count this PR as a 6/7 regression, because this test-case has 
been failing since fopenmp was introduced in 4.2. Still, I think it 
makes sense to backport because this is a correctness fix.

OK to backport to 6/7 branch?

Thanks,
- Tom

On Sun, Dec 10, 2017 at 03:00:24PM +0100, Tom de Vries wrote:
> On 09/14/2017 12:55 PM, Jakub Jelinek wrote:
> > Ok with those changes
> 
> Hi,
> 
> this PR has been fixed on trunk (
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=252873 ).
> 
> We cannot count this PR as a 6/7 regression, because this test-case has been
> failing since fopenmp was introduced in 4.2. Still, I think it makes sense
> to backport because this is a correctness fix.
> 
> OK to backport to 6/7 branch?

Ok, thanks.

	Jakub