Thomas Schwinge <tho...@codesourcery.com> writes:
Hi Thomas,
> Via <https://gcc.gnu.org/PR94629> "10 issues located by the PVS-studio
> static analyzer" (so please reference that one on any patch submission),
> on <https://habr.com/en/company/pvs-studio/blog/497640/> in "Fragment N3,
> Assigning a variable to itself", we find this latter assignment qualified
> as "very strange to assign a variable to itself".
>
> Probably that should've been 'outer_ctx' instead of 'ctx'?
I agree that the original intention must have been to assign the
outer_ctx's "outer_reduction_clauses" to the corresponding field of the
inner "ctx". This would make sense, semantically. But this field is
meant to be used by the function "scan_omp_for" only and ...
> then does the current algorith still work despite this error?
... this function never requires the struct field to be intialized in
that way. Before the field is used, it always copies the clauses from
the outer context's outer_reduction_clauses to ctx->outer_reduction_clauses:
>> + if (ctx->outer_reduction_clauses == NULL && ctx->outer != NULL)
>> + ctx->outer_reduction_clauses
>> + = chainon (unshare_expr (ctx->outer->local_reduction_clauses),
>> + ctx->outer->outer_reduction_clauses);
Hence I found it preferrable to remove the assignment to the
"outer_reduction_clauses" field and the "local_reduction_clauses" field
from "new_omp_context" completely. (The fields are still zero intialized
by the allocation of the struct which uses XCNEW.) That way the whole
logic regarding the fields is now contained in "scan_omp_for".
I have executed "make check" (on x86_64-linux-gnu) to verify that the
change causes no regressions. Ok to push the commit to master?
Best regards,
Frederik
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
>From 2d60b374a44b212ff97c8b1fd6f8c39e478dc70f Mon Sep 17 00:00:00 2001
From: Frederik Harwath <frede...@codesourcery.com>
Date: Tue, 21 Apr 2020 12:36:14 +0200
Subject: [PATCH] Remove fishy self-assignment in omp-low.c [PR94629]
The PR noticed that omp-low.c contains a self-assignment in the
function new_omp_context:
if (outer_ctx) {
...
ctx->outer_reduction_clauses = ctx->outer_reduction_clauses;
This is obviously useless. The original intention might have been
to copy the field from the outer_ctx to ctx. Since this is done
(properly) in the only function where this field is actually used
(in function scan_omp_for) and the field is being initialized to zero
during the struct allocation, there is no need to attempt to do
anything to this field in new_omp_context. Thus this commit
removes any assignment to the field from new_omp_context.
2020-04-21 Frederik Harwath <frede...@codesourcery.com>
PR other/94629
* gcc/omp-low.c (new_omp_context): Remove assignments to
ctx->outer_reduction_clauses and ctx->local_reduction_clauses.
---
gcc/omp-low.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 67565d61400..88f23e60d34 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -128,10 +128,16 @@ struct omp_context
corresponding tracking loop iteration variables. */
hash_map<tree, tree> *lastprivate_conditional_map;
- /* A tree_list of the reduction clauses in this context. */
+ /* A tree_list of the reduction clauses in this context. This is
+ only used for checking the consistency of OpenACC reduction
+ clauses in scan_omp_for and is not guaranteed to contain a valid
+ value outside of this function. */
tree local_reduction_clauses;
- /* A tree_list of the reduction clauses in outer contexts. */
+ /* A tree_list of the reduction clauses in outer contexts. This is
+ only used for checking the consistency of OpenACC reduction
+ clauses in scan_omp_for and is not guaranteed to contain a valid
+ value outside of this function. */
tree outer_reduction_clauses;
/* Nesting depth of this context. Used to beautify error messages re
@@ -931,8 +937,6 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx)
ctx->outer = outer_ctx;
ctx->cb = outer_ctx->cb;
ctx->cb.block = NULL;
- ctx->local_reduction_clauses = NULL;
- ctx->outer_reduction_clauses = ctx->outer_reduction_clauses;
ctx->depth = outer_ctx->depth + 1;
}
else
@@ -948,8 +952,6 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx)
ctx->cb.transform_call_graph_edges = CB_CGE_MOVE;
ctx->cb.adjust_array_error_bounds = true;
ctx->cb.dont_remap_vla_if_no_change = true;
- ctx->local_reduction_clauses = NULL;
- ctx->outer_reduction_clauses = NULL;
ctx->depth = 1;
}
--
2.17.1
