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.