diff mbox series

OpenMP: Ignore side-effects when finding struct comps [PR108545]

Message ID 437597e7-a9ee-f6f1-3490-dd4e75ee13de@codesourcery.com
State New
Headers show
Series OpenMP: Ignore side-effects when finding struct comps [PR108545] | expand

Commit Message

Tobias Burnus Feb. 28, 2023, 1:06 p.m. UTC 
(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

Comments

Jakub Jelinek March 1, 2023, 1:03 p.m. UTC | #1 
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
Tobias Burnus March 1, 2023, 1:48 p.m. UTC | #2 
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 <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.
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
diff mbox series

Patch

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 <tree_node>
+{
+  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 &t)
+{
+  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.  */
 
@@ -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<tree_operand_hash,
+omp_index_mapping_groups_1 (hash_map<tree_operand_sideeff_hash,
 				     omp_mapping_group *> *grpmap,
 			    vec<omp_mapping_group> *groups,
 			    tree reindex_sentinel)
@@ -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
@@ -9478,11 +9499,11 @@  omp_index_mapping_groups_1 (hash_map<tree_operand_hash,
     }
 }
 
-static hash_map<tree_operand_hash, omp_mapping_group *> *
+static hash_map<tree_operand_sideeff_hash, omp_mapping_group *> *
 omp_index_mapping_groups (vec<omp_mapping_group> *groups)
 {
-  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap
-    = new hash_map<tree_operand_hash, omp_mapping_group *>;
+  hash_map<tree_operand_sideeff_hash, omp_mapping_group *> *grpmap
+    = new hash_map<tree_operand_sideeff_hash, omp_mapping_group *>;
 
   omp_index_mapping_groups_1 (grpmap, groups, NULL_TREE);
 
@@ -9502,14 +9523,14 @@  omp_index_mapping_groups (vec<omp_mapping_group> *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<tree_operand_hash, omp_mapping_group *> *
+static hash_map<tree_operand_sideeff_hash, omp_mapping_group *> *
 omp_reindex_mapping_groups (tree *list_p,
 			    vec<omp_mapping_group> *groups,
 			    vec<omp_mapping_group> *processed_groups,
 			    tree sentinel)
 {
-  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap
-    = new hash_map<tree_operand_hash, omp_mapping_group *>;
+  hash_map<tree_operand_sideeff_hash, omp_mapping_group *> *grpmap
+    = new hash_map<tree_operand_sideeff_hash, omp_mapping_group *>;
 
   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<tree_operand_hash,
+omp_mapped_by_containing_struct (hash_map<tree_operand_sideeff_hash,
 					  omp_mapping_group *> *grpmap,
 				 tree decl,
 				 omp_mapping_group **mapped_by_group)
@@ -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 *>
 			      *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 *>
 			    *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<omp_mapping_group> *groups,
-				  hash_map<tree_operand_hash,
+				  hash_map<tree_operand_sideeff_hash,
 					   omp_mapping_group *> *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<omp_mapping_group> *groups,
-				hash_map<tree_operand_hash, omp_mapping_group *>
-				  **grpmap,
+				hash_map<tree_operand_sideeff_hash,
+					 omp_mapping_group *> **grpmap,
 				tree *list_p)
 {
   unsigned i;
@@ -10747,7 +10768,7 @@  gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
       groups = omp_gather_mapping_groups (list_p);
       if (groups)
 	{
-	  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap;
+	  hash_map<tree_operand_sideeff_hash, omp_mapping_group *> *grpmap;
 	  grpmap = omp_index_mapping_groups (groups);
 
 	  omp_build_struct_sibling_lists (code, region_type, groups, &grpmap,
@@ -10783,7 +10804,7 @@  gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
       groups = omp_gather_mapping_groups (list_p);
       if (groups)
 	{
-	  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap;
+	  hash_map<tree_operand_sideeff_hash, omp_mapping_group *> *grpmap;
 	  grpmap = omp_index_mapping_groups (groups);
 
 	  oacc_resolve_clause_dependencies (groups, grpmap);
diff --git a/gcc/testsuite/c-c++-common/gomp/map-8.c b/gcc/testsuite/c-c++-common/gomp/map-8.c
new file mode 100644
index 00000000000..cc7dac8bd0f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/map-8.c
@@ -0,0 +1,19 @@ 
+/* { dg-additional-options "-fdump-tree-omplower" } */
+
+/* PR fortran/108545 */
+
+/* { dg-final { scan-tree-dump "#pragma omp target enter data map\\(struct:my_struct \\\[len: 1\\\]\\) map\\(to:my_struct.u \\\[len: \[0-9\]+\\\]\\)" "omplower" } } */
+/* { dg-final { scan-tree-dump "#pragma omp target enter data map\\(to:my_struct3 \\\[len: \[0-9\]+\\\]\\)" "omplower" } } */
+
+
+volatile struct t {
+  struct t2 { int *a; int c; } u;
+  int b;
+} my_struct;
+volatile struct t3 { int *a; int c; } my_struct3;
+
+void f()
+{
+  #pragma omp target enter data map(to:my_struct.u) map(to:my_struct.u.a)
+  #pragma omp target enter data map(to:my_struct3) map(to:my_struct3.a)
+}
diff --git a/gcc/testsuite/gfortran.dg/gomp/map-9.f90 b/gcc/testsuite/gfortran.dg/gomp/map-9.f90
new file mode 100644
index 00000000000..9e7b811c8af
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/map-9.f90
@@ -0,0 +1,13 @@ 
+! { dg-additional-options "-fdump-tree-omplower" }
+
+! PR fortran/108545
+
+! { dg-final { scan-tree-dump "#pragma omp target enter data map\\(struct:x \\\[len: 1\\\]\\) map\\(to:x.a \\\[len: \[0-9\]+\\\]\\) map\\(to:MEM <integer\\(kind=4\\)\\\[0:\\\]> \\\[\\(integer\\(kind=4\\)\\\[0:\\\] \\*\\)_\[0-9\]+] \\\[len: _\[0-9\]+\\\]\\) map\\(always_pointer:x.a.data \\\[pointer assign, bias: 0\\\]\\)" "omplower" } }
+
+program p
+   type t
+      integer, pointer :: a(:)
+   end type
+   type(t), volatile :: x
+   !$omp target enter data map(to: x%a)
+end