On Tue, Feb 28, 2023 at 02:06:43PM +0100, Tobias Burnus wrote:
> (This is marked as P1 regression)
>
> Since the structure handling updates, a hash is now used to find expressions
> which are identical;
> unfortunately, this mishandles 'volatile' vars as expressions involving them
> are not regarded
> as identical. This leads to spurious *multiple* 'map(struct:x' that later
> causes an ICE.
> (For details, see also the PR, https://gcc.gnu.org/PR108545 )
Do we use that hashing even say for ARRAY_REFs with array indexes?
Using OEP_MATCH_SIDE_EFFECTS will mean that
volatile int idx;
a.b[idx] and a.b[idx] will compare equal. On the other side,
if idx yielded different values on the same construct between the two,
I think it would be invalid OpenMP (trying to map the two different
array elements from the same array section), so perhaps we are ok.
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -8958,6 +8958,28 @@ enum omp_tsort_mark {
> PERMANENT
> };
>
> +/* Hash for trees based on operand_equal_p.
> + Like tree_operand_hash but accepts side effects. */
> +struct tree_operand_sideeff_hash : ggc_ptr_hash <tree_node>
Not sure about the name, it isn't about that it accepts side effects,
but that it ignores them in the equality comparisons.
OEP_MATCH_SIDE_EFFECTS is probably misnamed too.
So perhaps struct tree_operand_hash_no_se (+ adjust the comment)?
Also, can't you derive it from tree_operand_hash?
Than you wouldn't need to provide your own hash, you could
inherit tree_operand_hash::hash and just override equal.
> + return iterative_hash_expr (t, 0);
> +}
> +
> +inline bool
> +tree_operand_sideeff_hash::equal (const value_type &t1,
> + const compare_type &t2)
> +{
> + return operand_equal_p (t1, t2, OEP_MATCH_SIDE_EFFECTS);
> +}
> +
> /* A group of OMP_CLAUSE_MAP nodes that correspond to a single "map"
> clause. */
>
> @@ -9432,7 +9454,6 @@ omp_index_mapping_groups_1 (hash_map<tree_operand_hash,
> node = OMP_CLAUSE_CHAIN (node), j++)
> {
> tree decl = OMP_CLAUSE_DECL (node);
> -
> /* Sometimes we see zero-offset MEM_REF instead of INDIRECT_REF,
> meaning node-hash lookups don't work. This is a workaround for
> that, but ideally we should just create the INDIRECT_REF at
Why? No change around this and I think it is nicer to have empty line
separating start of block declarations from the rest.
> @@ -9590,7 +9611,7 @@ omp_mapped_by_containing_struct
> (hash_map<tree_operand_hash,
> static bool
> omp_tsort_mapping_groups_1 (omp_mapping_group ***outlist,
> vec<omp_mapping_group> *groups,
> - hash_map<tree_operand_hash, omp_mapping_group *>
> + hash_map<tree_operand_sideeff_hash,
> omp_mapping_group *>
This is too long, even with the shorter name.
> *grpmap,
> omp_mapping_group *grp)
> {
> @@ -9670,7 +9691,7 @@ omp_tsort_mapping_groups_1 (omp_mapping_group
> ***outlist,
>
> static omp_mapping_group *
> omp_tsort_mapping_groups (vec<omp_mapping_group> *groups,
> - hash_map<tree_operand_hash, omp_mapping_group *>
> + hash_map<tree_operand_sideeff_hash, omp_mapping_group
> *>
This could fit after the change though.
Otherwise LGTM.
Jakub
