Re: [Patch] OpenMP: Ignore side-effects when finding struct comps [PR108545]
On 01.03.23 14:03, Jakub Jelinek wrote:
On Tue, Feb 28, 2023 at 02:06:43PM +0100, Tobias Burnus wrote:
[...]
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.
I think we are also okay because the sorting is for grouping and not
for combining. I think you mean code like:
struct t { int b[10]; } a;
void f() {
volatile int i = 1;
#pragma omp target enter data map(to: a.b[i], a.b[i])
}
This produces (independent of the patch) in the omplower dump:
#pragma omp target enter data \
map(struct:a [len: 2]) \
map(to:a.b[i.0] [len: 4]) \
map(to:a.b[i.1] [len: 4])
(Same result when removing the 'volatile' on 'i'.)
Without the patch, adding the 'volatile' to 'a' will break the
analysis such that two 'struct:a [len: 1]' appear – causing the ICE.
With the patch, we are back to the initial result as shown above.
+/* Hash for trees based on operand_equal_p.
+ Like tree_operand_hash but accepts side effects. */
+struct tree_operand_sideeff_hash : ggc_ptr_hash
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.
I concur.
So perhaps struct tree_operand_hash_no_se (+ adjust the comment)?
OK.
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.
That would work as well - and done so.
Otherwise LGTM.
Updated version attached. Once build + quick test has succeeded, I
intent to commit it, unless more comments come up.
Thanks for the review/suggestions,
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
OpenMP: Ignore side-effects when finding struct comps [PR108545]
With volatile, two 'x.data' comp refs aren't regarded as identical,
causing that the two items in the first map of
map(to:x.a, x.a.data) map(pset: x.a.data)
end up in separate 'map(struct:x)', which will cause a later ICE.
Solution: Ignore side effects when checking the operands in the hash
for being equal. (Do so by creating a variant of tree_operand_hash
that calls operand_equal_p with OEP_MATCH_SIDE_EFFECTS.)
gcc/ChangeLog:
PR middle-end/108545
* gimplify.cc (struct tree_operand_hash_no_se): New.
omp_index_mapping_groups_1, omp_index_mapping_groups,
omp_reindex_mapping_groups, omp_mapped_by_containing_struct,
omp_tsort_mapping_groups_1, omp_tsort_mapping_groups,
oacc_resolve_clause_dependencies, omp_build_struct_sibling_lists,
gimplify_scan_omp_clauses): Use tree_operand_hash_no_se instead
of tree_operand_hash.
gcc/testsuite/ChangeLog:
PR middle-end/108545
* c-c++-common/gomp/map-8.c: New test.
* gfortran.dg/gomp/map-9.f90: New test.
gcc/gimplify.cc | 51 +---
gcc/testsuite/c-c++-common/gomp/map-8.c | 19
gcc/testsuite/gfortran.dg/gomp/map-9.f90 | 13
3 files changed, 65 insertions(+), 18 deletions(-)
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 35d1ea22623..ade6e335da7 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -8958,6 +8958,22 @@ enum omp_tsort_mark {
PERMANENT
};
+/* Hash for trees based on operand_equal_p. Like tree_operand_hash
+ but ignores side effects in the equality comparisons. */
+
+struct tree_operand_hash_no_se : tree_operand_hash
+{
+ static inline bool equal (const value_type &,
+ const compare_type &);
+};
+
+inline bool
+tree_operand_hash_no_se::equal (const value_type ,
+const compare_type )
+{
+ return operand_equal_p (t1, t2, OEP_MATCH_SIDE_EFFECTS);
+}
+
/* A group of OMP_CLAUSE_MAP nodes that correspond to a single "map"
clause. */
@@ -9400,10 +9416,10 @@ omp_group_base (omp_mapping_group *grp, unsigned int *chained,
}
/* Given a vector of omp_mapping_groups, build a hash table so we can look up
- nodes by tree_operand_hash. */
+ nodes by tree_operand_hash_no_se. */
static void
-omp_index_mapping_groups_1 (hash_map *grpmap,
vec *groups,
tree reindex_sentinel)
@@ -9432,7 +9448,6 @@ omp_index_mapping_groups_1 (hash_map *
+static hash_map *
omp_index_mapping_groups (vec *groups)
{
- hash_map *grpmap
-= new hash_map;
+ hash_map *grpmap
+= new hash_map;
omp_index_mapping_groups_1 (grpmap, groups, NULL_TREE);
@@ -9502,14 +9517,14 @@ omp_index_mapping_groups (vec *groups)
so, we can do the reindex operation in
Re: [Patch] OpenMP: Ignore side-effects when finding struct comps [PR108545]
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
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 ,
> + const compare_type )
> +{
> + 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 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 static bool
> omp_tsort_mapping_groups_1 (omp_mapping_group ***outlist,
> vec *groups,
> - hash_map
> + hash_map 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 *groups,
> - hash_map
> + hash_map *>
This could fit after the change though.
Otherwise LGTM.
Jakub
[Patch] OpenMP: Ignore side-effects when finding struct comps [PR108545]
(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 )
OK for mainline?
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
OpenMP: Ignore side-effects when finding struct comps [PR108545]
With volatile, two 'x.data' comp refs aren't regarded as identical,
causing that the two items in the first map of
map(to:x.a, x.a.data) map(pset: x.a.data)
end up in separate 'map(struct:x)', which will cause a later ICE.
Solution: Ignore side effects when checking the operands in the hash
for being equal. (Do so by creating a variant of tree_operand_hash
that calls operand_equal_p with OEP_MATCH_SIDE_EFFECTS.)
gcc/ChangeLog:
PR middle-end/108545
* gimplify.cc (struct tree_operand_sideeff_hash): New.
omp_index_mapping_groups_1, omp_index_mapping_groups,
omp_reindex_mapping_groups, omp_mapped_by_containing_struct,
omp_tsort_mapping_groups_1, omp_tsort_mapping_groups,
oacc_resolve_clause_dependencies, omp_build_struct_sibling_lists,
gimplify_scan_omp_clauses): Use tree_operand_sideeff_hash instead
of tree_operand_hash.
gcc/testsuite/ChangeLog:
PR middle-end/108545
* c-c++-common/gomp/map-8.c: New test.
* gfortran.dg/gomp/map-9.f90: New test.
gcc/gimplify.cc | 55 ++--
gcc/testsuite/c-c++-common/gomp/map-8.c | 19 +++
gcc/testsuite/gfortran.dg/gomp/map-9.f90 | 13
3 files changed, 70 insertions(+), 17 deletions(-)
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 96845154a92..3c0bb1c987a 100644
--- 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
+{
+ static inline hashval_t hash (const value_type &);
+ static inline bool equal (const value_type &,
+ const compare_type &);
+};
+
+inline hashval_t
+tree_operand_sideeff_hash::hash (const value_type )
+{
+ return iterative_hash_expr (t, 0);
+}
+
+inline bool
+tree_operand_sideeff_hash::equal (const value_type ,
+ const compare_type )
+{
+ return operand_equal_p (t1, t2, OEP_MATCH_SIDE_EFFECTS);
+}
+
/* A group of OMP_CLAUSE_MAP nodes that correspond to a single "map"
clause. */
@@ -9400,10 +9422,10 @@ omp_group_base (omp_mapping_group *grp, unsigned int *chained,
}
/* Given a vector of omp_mapping_groups, build a hash table so we can look up
- nodes by tree_operand_hash. */
+ nodes by tree_operand_sideeff_hash. */
static void
-omp_index_mapping_groups_1 (hash_map *grpmap,
vec *groups,
tree reindex_sentinel)
@@ -9432,7 +9454,6 @@ omp_index_mapping_groups_1 (hash_map *
+static hash_map *
omp_index_mapping_groups (vec *groups)
{
- hash_map *grpmap
-= new hash_map;
+ hash_map *grpmap
+= new hash_map;
omp_index_mapping_groups_1 (grpmap, groups, NULL_TREE);
@@ -9502,14 +9523,14 @@ omp_index_mapping_groups (vec *groups)
so, we can do the reindex operation in two parts, on the processed and
then the unprocessed halves of the list. */
-static hash_map *
+static hash_map *
omp_reindex_mapping_groups (tree *list_p,
vec *groups,
vec *processed_groups,
tree sentinel)
{
- hash_map *grpmap
-= new hash_map;
+ hash_map *grpmap
+= new hash_map;
processed_groups->truncate (0);
@@ -9550,7 +9571,7 @@ omp_containing_struct (tree expr)
that maps that structure, if present. */
static bool
-omp_mapped_by_containing_struct (hash_map *grpmap,
tree decl,
omp_mapping_group **mapped_by_group)
@@ -9590,7 +9611,7 @@ omp_mapped_by_containing_struct (hash_map *groups,
- hash_map
+ hash_map
*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 *groups,
- hash_map
+ hash_map
*grpmap)
{
omp_mapping_group *grp, *outlist = NULL, **cursor;
@@ -9986,7 +10007,7 @@ omp_check_mapping_compatibility (location_t loc,
void
oacc_resolve_clause_dependencies (vec *groups,
- hash_map *grpmap)
{
int i;
@@ -10520,8 +10541,8 @@ static bool
omp_build_struct_sibling_lists (enum tree_code code,
enum omp_region_type region_type,
vec *groups,
-hash_map
- **grpmap,
+hash_map **grpmap,
tree *list_p)
{
unsigned i;
@@ -10747,7 +10768,7 @@
