From patchwork Wed Feb 28 19:32:44 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Frederik Harwath X-Patchwork-Id: 1905983 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=harwath.name header.i=frederik@harwath.name header.a=rsa-sha256 header.s=s1-ionos header.b=tYE5Tx7Y; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4TlQ0n6x9lz1yX7 for ; Thu, 29 Feb 2024 06:49:29 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 00872385829E for ; Wed, 28 Feb 2024 19:49:28 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.134]) by sourceware.org (Postfix) with ESMTPS id 3EC3038582B1 for ; Wed, 28 Feb 2024 19:32:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3EC3038582B1 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=harwath.name Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=harwath.name ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 3EC3038582B1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=212.227.126.134 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709148776; cv=none; b=TqrFyZslYamzlkhl2WRkYxR0H9MyxQwRzornn7cVviCu7XpzxB4l/JR/S71DmHK/0OelFj71etoRqrfxaWVtWaLLViVO/ybo7SDqdbflyYv0PmV7YYeFbE5067itefg3tN9EYZSfBDOQ0qYPf2m5eBN5Q9PYAt4lLwJrs8cuMRk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709148776; c=relaxed/simple; bh=72Pu4U7LblTINwYc1R5X67JM/0hoUmP7rgNSOevK7R4=; h=DKIM-Signature:Message-ID:Date:MIME-Version:To:From:Subject; b=gq/CnWr95fob8/Q9MxcujtiHKIGUi5WN7Pq/V6XFl2QJfdRXZP66WYGtZlUf8jUm2ExXFn7qs45hmnxYDNWl0cf8V7Nf9LgukcYMoSEafGkMWjJPBSHloDrw1LXg1ex5aFMaS7lQsA9Mx5lfO1Nr6MyIETBwiRzw9YJB7QVNlmk= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=harwath.name; s=s1-ionos; t=1709148764; x=1709753564; i=frederik@harwath.name; bh=72Pu4U7LblTINwYc1R5X67JM/0hoUmP7rgNSOevK7R4=; h=X-UI-Sender-Class:Date:To:From:Subject; b=tYE5Tx7YkyvzPxuyqt9i65YU1BW16GPssTdlNkUwYsDXrU8vz8dW9buVg6SdYYrm p0kT0oTxZjfLTUy4sPtscMXYqAwKuiHzvju+HzqFZa0ZgFRITpHvA32bZ5RkJ1eur 4H3rt2FfuSYnYT6EwFKD/6ZWAUQaa5TZN3D0eYLJoEIZnoOjNQSV5VGae12Syd7I2 nq5fXo1Za4DiKwz6Icls+P9oZ87D6U76kSL4W3UjFx/wDR+KNJDz6yQqhjPTbRznq 2cEXoyuXET5aplH0Zmc0vmG8Gzd2yrMgSufr1gvJHKrMub06kBaWGqzW5qTGaiXIF IBHaGBpLLt2GnR35rQ== X-UI-Sender-Class: 55c96926-9e95-11ee-ae09-1f7a4046a0f6 Received: from [192.168.178.47] ([217.245.93.161]) by mrelayeu.kundenserver.de (mreue012 [213.165.67.97]) with ESMTPSA (Nemesis) id 1N0qmr-1qinOr26ib-00wo49; Wed, 28 Feb 2024 20:32:44 +0100 Message-ID: Date: Wed, 28 Feb 2024 20:32:44 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: gcc-patches@gcc.gnu.org, tburnus@baylibre.com, jakub@redhat.com From: Frederik Harwath Subject: [PATCH] OpenMP: warn about iteration var modifications in loop body X-Provags-ID: V03:K1:1Mtb1ze2Pd1n/VrRITClZl4eYaMkFNezZsor7w00owVXlyP8JKN nnuTn/Y2NEB5ispDXtELKP8VceqfjXpwtQF2+qnfY+i2jqaO5AIg1yDJW86KxDTCz4SCBYf 3Ip+mjsJKXR/DWS/iza1xyVvtLhYEgingBYRbL18EwRnmyk/zGD+esI3Qfm7EXK3J8RlPne u+15AjBq/buyCpZ1TCK0g== UI-OutboundReport: notjunk:1;M01:P0:9p0d2ojU8aQ=;9vN+QousdBVBkq53ayh1uWxoNkd 5HFRpd/mcKstnlDXIR1kUz8y8B33bYycSmjwKAAsiv7pXlyNIKT3vaTtBckfmLpMnn9tekRIW QCM93PWS3ufzwI1Sp7vTIrsDIyHnCajmf55v7fCKfjgDOzou8iQciNS/hprRW7BgpjdhZkmXZ qu9cN1w9S6NZbDfLaeaHPgcsck5LUufcC5ZXvfNr6P4B36A+x9mFzt4b7DQpqazWVcWshAq+V V7nD0aj9F2dZ2mbnxbv75eT+8PbWRPfv1PY+fVZrb1fnqlWMRnhVRgOTBK/lDIbDMi9WenFTR F59tywugdkPpHFQTMMk1cvvis0Xwih6n6fzU31sJ1vxQmBWfZNUBn9I9wnQoQq24tt0wEeUmj 0ufedI/MTDxHRY22uEmxI328xmrq0uRLQkQgz38Bmty9P0qdqAb1fra/lGXkoH8vpHOKgU6Ul Gcg7+kSmFPFOSdqFlkQGz0oQw9F8R9PIdGbaO9/z++NAkWv2qWlbRNCgPjKaqct5qx5RXXn10 lFqGetT+DaU0GRsW3GTAINu5XbCEA+lSwhKmmxsCMuR63dfX6yCrOU5Q6p0SQ8NneYGw187jC e59cf4R+Dgeh404fFUMO90yWiDCBXuAFFgVLmjAQdc7S8UF8QtiO5HgRVFQD/FVp/RrC7SK0x aiGPLt+0wr6wSFY3jixsUcZFOCjxJuUwh9GjV2uq/3m6KVRdXnGl3k7YoPB97rbTv9U2Zb7nf xYpMR4JfO89YOH9CZ1UokHCxBkT50Rn/3IjcOi2W+GwrCfboCziVUE= X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Hi, this patch implements a warning about (some simple cases of direct) modifications of iteration variables in OpenMP loops which are forbidden according to the OpenMP specification. I think this can be helpful, especially for new OpenMP users. I have implemented this after I observed some confusion concerning this topic recently. The check is implemented during gimplification. It reuses the "loop_iter_var" vector in the "gimplify_omp_ctx" which was previously only used for "doacross" handling to identify the loop iteration variables during the gimplification of MODIFY_EXPRs in omp_for bodies. I have only added a common C/C++ test because I don't see any special C++ constructs for which a warning *should* be emitted and Fortran rejects modifications of iteration variables in do loops in general. I have run "make check" on x86_64-linux-gnu and not observed any regressions. Is it ok to commit this? Best regards, Frederik From 4944a9f94bcda9907e0118e71137ee7e192657c2 Mon Sep 17 00:00:00 2001 From: Frederik Harwath Date: Tue, 27 Feb 2024 21:07:00 +0000 Subject: [PATCH] OpenMP: warn about iteration var modifications in loop body OpenMP loop iteration variables may not be changed by user code in the loop body according to the OpenMP specification. In general, the compiler cannot enforce this, but nevertheless simple cases in which the user modifies the iteration variable directly in the loop body (in contrast to, e.g., modifications through a pointer) can be recognized. A warning should be useful, for instance, to new users of OpenMP. This commit implements a warning about forbidden iteration var modifications during gimplification. It reuses the "loop_iter_var" vector in the "gimplify_omp_ctx" which was previously only used for "doacross" handling to identify the loop iteration variables during the gimplification of MODIFY_EXPRs in omp_for bodies. gcc/ChangeLog: * gimplify.cc (struct gimplify_omp_ctx): Add field "in_omp_for_body" to recognize the gimplification state during which the new warning should be emitted. Add field "is_doacross" to distinguish the original use of "loop_iter_var" from its new use. (new_omp_context): Initialize new gimplify_omp_ctx fields. (gimplify_modify_expr): Emit warning if iter var is modified. (gimplify_omp_for): Make initialization and filling of loop_iter_var vector unconditional and adjust new gimplify_omp_ctx fields before gimplifying the omp_for body. (gimplify_omp_ordered): Check for do_across field in addition to emptiness check on loop_iter_var vector since the vector is now always being filled. gcc/testsuite/ChangeLog: * c-c++-common/gomp/iter-var-modification.c: New test. Signed-off-by: Frederik Harwath --- gcc/gimplify.cc | 54 +++++++--- .../c-c++-common/gomp/iter-var-modification.c | 100 ++++++++++++++++++ 2 files changed, 138 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/gomp/iter-var-modification.c diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc index 7f79b3cc7e6..a74ad987cf7 100644 --- a/gcc/gimplify.cc +++ b/gcc/gimplify.cc @@ -235,6 +235,8 @@ struct gimplify_omp_ctx bool order_concurrent; bool has_depend; bool in_for_exprs; + bool in_omp_for_body; + bool is_doacross; int defaultmap[5]; }; @@ -456,6 +458,10 @@ new_omp_context (enum omp_region_type region_type) c->privatized_types = new hash_set; c->location = input_location; c->region_type = region_type; + c->loop_iter_var.create (0); + c->in_omp_for_body = false; + c->is_doacross = false; + if ((region_type & ORT_TASK) == 0) c->default_kind = OMP_CLAUSE_DEFAULT_SHARED; else @@ -6312,6 +6318,18 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR || TREE_CODE (*expr_p) == INIT_EXPR); + if (gimplify_omp_ctxp && gimplify_omp_ctxp->in_omp_for_body) + { + size_t num_vars = gimplify_omp_ctxp->loop_iter_var.length () / 2; + for (size_t i = 0; i < num_vars; i++) + { + if (*to_p == gimplify_omp_ctxp->loop_iter_var[2 * i + 1]) + warning_at (input_location, OPT_Wopenmp, + "forbidden modification of iteration variable %qE in " + "OpenMP loop", *to_p); + } + } + /* Trying to simplify a clobber using normal logic doesn't work, so handle it here. */ if (TREE_CLOBBER_P (*from_p)) @@ -15334,6 +15352,8 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p) == TREE_VEC_LENGTH (OMP_FOR_COND (for_stmt))); gcc_assert (TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt)) == TREE_VEC_LENGTH (OMP_FOR_INCR (for_stmt))); + int len = TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt)); + gimplify_omp_ctxp->loop_iter_var.create (len * 2); tree c = omp_find_clause (OMP_FOR_CLAUSES (for_stmt), OMP_CLAUSE_ORDERED); bool is_doacross = false; @@ -15342,8 +15362,6 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p) { OMP_CLAUSE_ORDERED_DOACROSS (c) = 1; is_doacross = true; - int len = TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt)); - gimplify_omp_ctxp->loop_iter_var.create (len * 2); for (tree *pc = &OMP_FOR_CLAUSES (for_stmt); *pc; ) if (OMP_CLAUSE_CODE (*pc) == OMP_CLAUSE_LINEAR) { @@ -15380,23 +15398,22 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p) gcc_assert (DECL_P (decl)); gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (decl)) || POINTER_TYPE_P (TREE_TYPE (decl))); - if (is_doacross) + + if (TREE_CODE (for_stmt) == OMP_FOR && OMP_FOR_ORIG_DECLS (for_stmt)) { - if (TREE_CODE (for_stmt) == OMP_FOR && OMP_FOR_ORIG_DECLS (for_stmt)) + tree orig_decl = TREE_VEC_ELT (OMP_FOR_ORIG_DECLS (for_stmt), i); + if (TREE_CODE (orig_decl) == TREE_LIST) { - tree orig_decl = TREE_VEC_ELT (OMP_FOR_ORIG_DECLS (for_stmt), i); - if (TREE_CODE (orig_decl) == TREE_LIST) - { - orig_decl = TREE_PURPOSE (orig_decl); - if (!orig_decl) - orig_decl = decl; - } - gimplify_omp_ctxp->loop_iter_var.quick_push (orig_decl); + orig_decl = TREE_PURPOSE (orig_decl); + if (!orig_decl) + orig_decl = decl; } - else - gimplify_omp_ctxp->loop_iter_var.quick_push (decl); - gimplify_omp_ctxp->loop_iter_var.quick_push (decl); + gimplify_omp_ctxp->loop_iter_var.quick_push (orig_decl); } + else + gimplify_omp_ctxp->loop_iter_var.quick_push (decl); + gimplify_omp_ctxp->loop_iter_var.quick_push (decl); + if (for_stmt == orig_for_stmt) { @@ -15818,9 +15835,13 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p) TREE_SIDE_EFFECTS (OMP_FOR_BODY (orig_for_stmt)) = 1; } } + gimplify_omp_ctxp->in_omp_for_body = true; + gimplify_omp_ctxp->is_doacross = is_doacross; gimple *g = gimplify_and_return_first (OMP_FOR_BODY (orig_for_stmt), &for_body); + gimplify_omp_ctxp->in_omp_for_body = false; + gimplify_omp_ctxp->is_doacross = false; if (TREE_CODE (orig_for_stmt) == OMP_TASKLOOP || (loop_p && orig_for_stmt == for_stmt)) @@ -17430,7 +17451,8 @@ gimplify_omp_ordered (tree expr, gimple_seq body) { for (c = OMP_ORDERED_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN (c)) if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DOACROSS - && gimplify_omp_ctxp->loop_iter_var.is_empty ()) + && (!gimplify_omp_ctxp->is_doacross + || gimplify_omp_ctxp->loop_iter_var.is_empty ())) { error_at (OMP_CLAUSE_LOCATION (c), "% construct with %qs clause must be " diff --git a/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c b/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c new file mode 100644 index 00000000000..5662fce2a6f --- /dev/null +++ b/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c @@ -0,0 +1,100 @@ +extern int a[1000]; + +int main () +{ +#pragma omp for + for (int i = 0; i < 1000; i++) + { + if (i % 2 == 0) + i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */ + } + + #pragma omp for + for (int i = 0; i < 1000; i++) + { + if (i % 2 == 0); + else + i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */ + } + +#pragma omp for + for (int i = 0; i < 1000; i++) + { + i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */ + } + +#pragma omp for + for (int i = 0; i != 1000; i++) + { + i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */ + } + +#pragma omp for + for (int i = 1000; i > 0; i--) + { + i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */ + } + +#pragma omp for + for (int *p = (int*)&a; p < a + 1000; p++) + { + p = (int*)&a; /* { dg-warning {forbidden modification of iteration variable .p. in OpenMP loop} } */ + } + +#pragma omp for + for (int *p = (int*)&a; p < a + 1000; p++) + { + *p = 0; + } + +#pragma omp parallel for collapse(3) + for (int i = 0; i < 1000; i++) + for (int j = 0; j < 1000; j++) + for (int k = 0; k < 1000; k++) + + { + j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */ + } + +#pragma omp target teams distribute parallel for collapse(3) + for (int i = 0; i < 1000; i++) + for (int j = 0; j < 1000; j++) + for (int k = 0; k < 1000; k++) + + { + k++; /* { dg-warning {forbidden modification of iteration variable .k. in OpenMP loop} } */ + } + +#pragma omp parallel for collapse(2) + for (int i = 0; i < 1000; i++) + for (int j = 0; j < 1000; j++) + for (int k = 0; k < 1000; k++) + + { + j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */ + } + +#pragma omp target teams distribute parallel for collapse(2) + for (int i = 0; i < 1000; i++) + for (int j = 0; j < 1000; j++) + for (int k = 0; k < 1000; k++) + + { + k++; /* No error since third loop is not included in collapsed loop-nest. */ + } + + /* Only modifications in the body should be reported. Do not warn about + assignments to i,k in the pre-body. */ + int i = 0; + int k = 0; +#pragma omp target teams distribute parallel for collapse(3) + for (i = 1; i < 1000; i++) + for (int j = 0; j < 1000; j++) + for (k = 1; k < 1000; k++) + + { + j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */ + } + + return 0; +} -- 2.34.1