Re: [PATCH, libgomp] Fix chunk_size<1 for dynamic schedule

[Chung-Lin Tang <cltang@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

On 2022/6/28 10:06 PM, Jakub Jelinek wrote:
> On Thu, Jun 23, 2022 at 11:47:59PM +0800, Chung-Lin Tang wrote:
>> with the way that chunk_size < 1 is handled for gomp_iter_dynamic_next:
>>
>> (1) chunk_size <= -1: wraps into large unsigned value, seems to work though.
>> (2) chunk_size == 0:  infinite loop
>>
>> The (2) behavior is obviously not desired. This patch fixes this by changing
> 
> Why?  It is a user error, undefined behavior, we shouldn't slow down valid
> code for users who don't bother reading the standard.

This is loop init code, not per-iteration. The overhead really isn't that much.

The question should be, if GCC having infinite loop behavior is reasonable,
even if it is undefined in the spec.

> E.g. OpenMP 5.1 [132:14] says clearly:
> "chunk_size must be a loop invariant integer expression with a positive
> value."
> and omp_set_schedule for chunk_size < 1 should use a default value (which it
> does).
> 
> For OMP_SCHEDULE the standard says it is implementation-defined what happens
> if the format isn't the specified one, so I guess the env.c change
> could be acceptable (though without it it is fine too), but the
> loop.c change is wrong.  Note, if the loop.c change would be ok, you'd
> need to also change loop_ull.c too.

I've updated the patch to add the same changes for libgomp/loop_ull.c and updated
the testcase too. Tested on mainline trunk without regressions.

Thanks,
Chung-Lin

libgomp/ChangeLog:

	* env.c (parse_schedule): Make negative values invalid for chunk_size.
	* loop.c (gomp_loop_init): For non-STATIC schedule and chunk_size <= 0,
	set initialized chunk_size to 1.
	* loop_ull.c (gomp_loop_ull_init): Likewise.

	* testsuite/libgomp.c/loop-28.c: New test.

[-- Attachment #2: chunk_size-v2.patch --]
[-- Type: text/plain, Size: 2091 bytes --]

diff --git a/libgomp/env.c b/libgomp/env.c
index 1c4ee894515..dff07617e15 100644
--- a/libgomp/env.c
+++ b/libgomp/env.c
@@ -182,6 +182,8 @@ parse_schedule (void)
     goto invalid;
 
   errno = 0;
+  if (*env == '-')
+    goto invalid;
   value = strtoul (env, &end, 10);
   if (errno || end == env)
     goto invalid;
diff --git a/libgomp/loop.c b/libgomp/loop.c
index be85162bb1e..018b4e9a8bd 100644
--- a/libgomp/loop.c
+++ b/libgomp/loop.c
@@ -41,7 +41,7 @@ gomp_loop_init (struct gomp_work_share *ws, long start, long end, long incr,
 		enum gomp_schedule_type sched, long chunk_size)
 {
   ws->sched = sched;
-  ws->chunk_size = chunk_size;
+  ws->chunk_size = (sched == GFS_STATIC || chunk_size > 1) ? chunk_size : 1;
   /* Canonicalize loops that have zero iterations to ->next == ->end.  */
   ws->end = ((incr > 0 && start > end) || (incr < 0 && start < end))
 	    ? start : end;
diff --git a/libgomp/loop_ull.c b/libgomp/loop_ull.c
index 602737296d4..74ddb1bd623 100644
--- a/libgomp/loop_ull.c
+++ b/libgomp/loop_ull.c
@@ -43,7 +43,7 @@ gomp_loop_ull_init (struct gomp_work_share *ws, bool up, gomp_ull start,
 		    gomp_ull chunk_size)
 {
   ws->sched = sched;
-  ws->chunk_size_ull = chunk_size;
+  ws->chunk_size_ull = (sched == GFS_STATIC || chunk_size > 1) ? chunk_size : 1;
   /* Canonicalize loops that have zero iterations to ->next == ->end.  */
   ws->end_ull = ((up && start > end) || (!up && start < end))
 		? start : end;
diff --git a/libgomp/testsuite/libgomp.c/loop-28.c b/libgomp/testsuite/libgomp.c/loop-28.c
new file mode 100644
index 00000000000..664842e27aa
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c/loop-28.c
@@ -0,0 +1,21 @@
+/* { dg-do run } */
+/* { dg-timeout 10 } */
+
+void __attribute__((noinline))
+foo (int a[], int n, int chunk_size)
+{
+  #pragma omp parallel for schedule (dynamic,chunk_size)
+  for (int i = 0; i < n; i++)
+    a[i] = i;
+
+  #pragma omp parallel for schedule (dynamic,chunk_size)
+  for (unsigned long long i = 0; i < n; i++)
+    a[i] = i;
+}
+
+int main (void)
+{
+  int a[100];
+  foo (a, 100, 0);
+  return 0;
+}