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.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;
+}
