public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][v2] Adjust volatile handling of the operand scanner
@ 2021-08-10 11:40 Richard Biener
  2021-08-10 15:55 ` Richard Biener
  2021-08-10 21:03 ` Eric Botcazou
  0 siblings, 2 replies; 8+ messages in thread
From: Richard Biener @ 2021-08-10 11:40 UTC (permalink / raw)
  To: gcc-patches; +Cc: jakub, ebotcazou, fortran

The GIMPLE SSA operand scanner handles COMPONENT_REFs that are
not marked TREE_THIS_VOLATILE but have a TREE_THIS_VOLATILE
FIELD_DECL as volatile.  That's inconsistent in how TREE_THIS_VOLATILE
testing on GENERIC refs works which requires operand zero of
component references to mirror TREE_THIS_VOLATILE to the ref
so that testing TREE_THIS_VOLATILE on the outermost reference
is enough to determine the volatileness.

The following patch thus removes FIELD_DECL scanning from
the GIMPLE SSA operand scanner, possibly leaving fewer stmts
marked as gimple_has_volatile_ops.

It shows we miss at least one case in the fortran frontend, though
there's a suspicious amount of COMPONENT_REF creation compared
to little setting of TREE_THIS_VOLATILE.  This fixes the FAIL
of gfortran.dg/volatile11.f90 that would otherwise occur.

Visually inspecting fortran/ reveals a bunch of likely to fix
cases but I don't know the constraints of 'volatile' uses in
the fortran language to assess whether some of these are not
necessary.  The cases would be fixed with

diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c
index 0d013defdbb..5d708fe90aa 100644
--- a/gcc/fortran/trans-array.c
+++ b/gcc/fortran/trans-array.c
@@ -6983,9 +6983,10 @@ gfc_get_dataptr_offset (stmtblock_t *block, tree parm, tree desc, tree offset,
 	    case REF_COMPONENT:
 	      field = ref->u.c.component->backend_decl;
 	      gcc_assert (field && TREE_CODE (field) == FIELD_DECL);
-	      tmp = fold_build3_loc (input_location, COMPONENT_REF,
-				     TREE_TYPE (field),
-				     tmp, field, NULL_TREE);
+	      tmp = build3_loc (input_location, COMPONENT_REF,
+				TREE_TYPE (field), tmp, field, NULL_TREE);
+	      if (TREE_THIS_VOLATILE (field))
+		TREE_THIS_VOLATILE (tmp) = 1;
 	      break;

 	    case REF_SUBSTRING:
diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
index c4291cce079..e6dc79f8c1e 100644
--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -2244,9 +2244,11 @@ gfc_get_tree_for_caf_expr (gfc_expr *expr)

 	if (POINTER_TYPE_P (TREE_TYPE (caf_decl)))
 	  caf_decl = build_fold_indirect_ref_loc (input_location, caf_decl);
-	caf_decl = fold_build3_loc (input_location, COMPONENT_REF,
-				    TREE_TYPE (comp->backend_decl), caf_decl,
-				    comp->backend_decl, NULL_TREE);
+	caf_decl = build3_loc (input_location, COMPONENT_REF,
+			       TREE_TYPE (comp->backend_decl), caf_decl,
+			       comp->backend_decl, NULL_TREE);
+	if (TREE_THIS_VOLATILE (comp->backend_decl))
+	  TREE_THIS_VOLATILE (caf_decl) = 1;
 	if (comp->ts.type == BT_CLASS)
 	  {
 	    caf_decl = gfc_class_data_get (caf_decl);
@@ -2755,8 +2757,10 @@ gfc_conv_component_ref (gfc_se * se, gfc_ref * ref)
   else
     se->class_vptr = NULL_TREE;

-  tmp = fold_build3_loc (input_location, COMPONENT_REF, TREE_TYPE (field),
-			 decl, field, NULL_TREE);
+  tmp =build3_loc (input_location, COMPONENT_REF, TREE_TYPE (field),
+		   decl, field, NULL_TREE);
+  if (TREE_THIS_VOLATILE (field))
+    TREE_THIS_VOLATILE (tmp) = 1;

   se->expr = tmp;

@@ -8792,8 +8796,10 @@ gfc_trans_structure_assign (tree dest, gfc_expr * expr, bool init, bool coarray)
 	}
       field = cm->backend_decl;
       gcc_assert(field);
-      tmp = fold_build3_loc (input_location, COMPONENT_REF, TREE_TYPE (field),
-			     dest, field, NULL_TREE);
+      tmp = build3_loc (input_location, COMPONENT_REF, TREE_TYPE (field),
+			dest, field, NULL_TREE);
+      if (TREE_THIS_VOLATILE (field))
+	TREE_THIS_VOLATILE (tmp) = 1;
       if (!c->expr)
 	{
 	  gfc_expr *e = gfc_get_null_expr (NULL);

but I did not include them as they have no effect on the testsuite.

The question is whether we instead want to amend build3 to
set TREE_THIS_VOLATILE automatically when the FIELD_DECL has
it set.  At least for the Fortran FE cases the gimplifier
fails to see some volatile references and thus can generate
wrong code which is a latent issue.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

OK?

Thanks,
Richard.

2021-08-09  Richard Biener  <rguenther@suse.de>

gcc/
	* tree-ssa-operands.c (operands_scanner::get_expr_operands):
	Do not look at COMPONENT_REF FIELD_DECLs TREE_THIS_VOLATILE
	to determine has_volatile_ops.

gcc/fortran/
	* trans-common.c (create_common): Set TREE_THIS_VOLATILE on the
	COMPONENT_REF if the field is volatile.
---
 gcc/fortran/trans-common.c | 9 +++++----
 gcc/tree-ssa-operands.c    | 7 +------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/gcc/fortran/trans-common.c b/gcc/fortran/trans-common.c
index a11cf4c839e..7bcf18dc475 100644
--- a/gcc/fortran/trans-common.c
+++ b/gcc/fortran/trans-common.c
@@ -759,10 +759,11 @@ create_common (gfc_common_head *com, segment_info *head, bool saw_equiv)
       else
 	gfc_add_decl_to_function (var_decl);
 
-      SET_DECL_VALUE_EXPR (var_decl,
-			   fold_build3_loc (input_location, COMPONENT_REF,
-					    TREE_TYPE (s->field),
-					    decl, s->field, NULL_TREE));
+      tree comp = build3_loc (input_location, COMPONENT_REF,
+			      TREE_TYPE (s->field), decl, s->field, NULL_TREE);
+      if (TREE_THIS_VOLATILE (s->field))
+	TREE_THIS_VOLATILE (comp) = 1;
+      SET_DECL_VALUE_EXPR (var_decl, comp);
       DECL_HAS_VALUE_EXPR_P (var_decl) = 1;
       GFC_DECL_COMMON_OR_EQUIV (var_decl) = 1;
 
diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
index c15575416dd..ebf7eea3b04 100644
--- a/gcc/tree-ssa-operands.c
+++ b/gcc/tree-ssa-operands.c
@@ -834,12 +834,7 @@ operands_scanner::get_expr_operands (tree *expr_p, int flags)
 	get_expr_operands (&TREE_OPERAND (expr, 0), flags);
 
 	if (code == COMPONENT_REF)
-	  {
-	    if (!(flags & opf_no_vops)
-		&& TREE_THIS_VOLATILE (TREE_OPERAND (expr, 1)))
-	      gimple_set_has_volatile_ops (stmt, true);
-	    get_expr_operands (&TREE_OPERAND (expr, 2), uflags);
-	  }
+	  get_expr_operands (&TREE_OPERAND (expr, 2), uflags);
 	else if (code == ARRAY_REF || code == ARRAY_RANGE_REF)
 	  {
 	    get_expr_operands (&TREE_OPERAND (expr, 1), uflags);
-- 
2.31.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][v2] Adjust volatile handling of the operand scanner
  2021-08-10 11:40 [PATCH][v2] Adjust volatile handling of the operand scanner Richard Biener
@ 2021-08-10 15:55 ` Richard Biener
  2021-08-10 21:03 ` Eric Botcazou
  1 sibling, 0 replies; 8+ messages in thread
From: Richard Biener @ 2021-08-10 15:55 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches, Jakub Jelinek, Eric Botcazou, fortran

On Tue, Aug 10, 2021 at 1:41 PM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The GIMPLE SSA operand scanner handles COMPONENT_REFs that are
> not marked TREE_THIS_VOLATILE but have a TREE_THIS_VOLATILE
> FIELD_DECL as volatile.  That's inconsistent in how TREE_THIS_VOLATILE
> testing on GENERIC refs works which requires operand zero of
> component references to mirror TREE_THIS_VOLATILE to the ref
> so that testing TREE_THIS_VOLATILE on the outermost reference
> is enough to determine the volatileness.
>
> The following patch thus removes FIELD_DECL scanning from
> the GIMPLE SSA operand scanner, possibly leaving fewer stmts
> marked as gimple_has_volatile_ops.
[...]

> The question is whether we instead want to amend build3 to
> set TREE_THIS_VOLATILE automatically when the FIELD_DECL has
> it set.

A change along that line also passes bootstrap and regtest.

Any preference?

Thanks,
Richard.

>  At least for the Fortran FE cases the gimplifier
> fails to see some volatile references and thus can generate
> wrong code which is a latent issue.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>
> OK?
>
> Thanks,
> Richard.
>
> 2021-08-09  Richard Biener  <rguenther@suse.de>
>
> gcc/
>         * tree-ssa-operands.c (operands_scanner::get_expr_operands):
>         Do not look at COMPONENT_REF FIELD_DECLs TREE_THIS_VOLATILE
>         to determine has_volatile_ops.
>
> gcc/fortran/
>         * trans-common.c (create_common): Set TREE_THIS_VOLATILE on the
>         COMPONENT_REF if the field is volatile.
> ---
>  gcc/fortran/trans-common.c | 9 +++++----
>  gcc/tree-ssa-operands.c    | 7 +------
>  2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/fortran/trans-common.c b/gcc/fortran/trans-common.c
> index a11cf4c839e..7bcf18dc475 100644
> --- a/gcc/fortran/trans-common.c
> +++ b/gcc/fortran/trans-common.c
> @@ -759,10 +759,11 @@ create_common (gfc_common_head *com, segment_info *head, bool saw_equiv)
>        else
>         gfc_add_decl_to_function (var_decl);
>
> -      SET_DECL_VALUE_EXPR (var_decl,
> -                          fold_build3_loc (input_location, COMPONENT_REF,
> -                                           TREE_TYPE (s->field),
> -                                           decl, s->field, NULL_TREE));
> +      tree comp = build3_loc (input_location, COMPONENT_REF,
> +                             TREE_TYPE (s->field), decl, s->field, NULL_TREE);
> +      if (TREE_THIS_VOLATILE (s->field))
> +       TREE_THIS_VOLATILE (comp) = 1;
> +      SET_DECL_VALUE_EXPR (var_decl, comp);
>        DECL_HAS_VALUE_EXPR_P (var_decl) = 1;
>        GFC_DECL_COMMON_OR_EQUIV (var_decl) = 1;
>
> diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
> index c15575416dd..ebf7eea3b04 100644
> --- a/gcc/tree-ssa-operands.c
> +++ b/gcc/tree-ssa-operands.c
> @@ -834,12 +834,7 @@ operands_scanner::get_expr_operands (tree *expr_p, int flags)
>         get_expr_operands (&TREE_OPERAND (expr, 0), flags);
>
>         if (code == COMPONENT_REF)
> -         {
> -           if (!(flags & opf_no_vops)
> -               && TREE_THIS_VOLATILE (TREE_OPERAND (expr, 1)))
> -             gimple_set_has_volatile_ops (stmt, true);
> -           get_expr_operands (&TREE_OPERAND (expr, 2), uflags);
> -         }
> +         get_expr_operands (&TREE_OPERAND (expr, 2), uflags);
>         else if (code == ARRAY_REF || code == ARRAY_RANGE_REF)
>           {
>             get_expr_operands (&TREE_OPERAND (expr, 1), uflags);
> --
> 2.31.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][v2] Adjust volatile handling of the operand scanner
  2021-08-10 11:40 [PATCH][v2] Adjust volatile handling of the operand scanner Richard Biener
  2021-08-10 15:55 ` Richard Biener
@ 2021-08-10 21:03 ` Eric Botcazou
  2021-08-11  6:40   ` Richard Biener
  1 sibling, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2021-08-10 21:03 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, jakub, fortran

> The question is whether we instead want to amend build3 to
> set TREE_THIS_VOLATILE automatically when the FIELD_DECL has
> it set.  At least for the Fortran FE cases the gimplifier
> fails to see some volatile references and thus can generate
> wrong code which is a latent issue.

What do we do for other similar flags, e.g. TREE_READONLY?

-- 
Eric Botcazou




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][v2] Adjust volatile handling of the operand scanner
  2021-08-10 21:03 ` Eric Botcazou
@ 2021-08-11  6:40   ` Richard Biener
  2021-08-11  8:29     ` Eric Botcazou
  2021-08-11  9:24     ` Richard Biener
  0 siblings, 2 replies; 8+ messages in thread
From: Richard Biener @ 2021-08-11  6:40 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: gcc-patches, jakub, fortran

On Tue, 10 Aug 2021, Eric Botcazou wrote:

> > The question is whether we instead want to amend build3 to
> > set TREE_THIS_VOLATILE automatically when the FIELD_DECL has
> > it set.  At least for the Fortran FE cases the gimplifier
> > fails to see some volatile references and thus can generate
> > wrong code which is a latent issue.
> 
> What do we do for other similar flags, e.g. TREE_READONLY?

build3 currently does no special processing for the FIELD_DECL operand,
it just sets TREE_THIS_VOLATILE from operand zero for tcc_references.

The C and C++ frontends have repeated patterns like

          ref = build3 (COMPONENT_REF, subtype, datum, subdatum,
                        NULL_TREE);
          SET_EXPR_LOCATION (ref, loc);
          if (TREE_READONLY (subdatum)
              || (use_datum_quals && TREE_READONLY (datum)))
            TREE_READONLY (ref) = 1;
          if (TREE_THIS_VOLATILE (subdatum)
              || (use_datum_quals && TREE_THIS_VOLATILE (datum)))
            TREE_THIS_VOLATILE (ref) = 1;

Leaving out TREE_READONLY shouldn't have any correctness issue.  It's
just that when adjusting the SSA operand scanner to correctly interpret
GENERIC that this uncovers pre-existing issues in the Fortran frontend
(one manifests in a testsuite FAIL - otherwise I wouldn't have noticed).

I'm fine with just plugging the Fortran FE holes as we discover them
but I did not check other frontends and testsuite coverage is weak.

Now - I wonder if there's a reason a frontend might _not_ want to
set TREE_THIS_VOLATILE on a COMPONENT_REF when the FIELD_DECL has
TREE_THIS_VOLATILE set.

I guess I'll do one more experiment and add verification that
TREE_THIS_VOLATILE on COMPONENT_REFs and FIELD_DECLs is consistent
and see where that trips.

Richard.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][v2] Adjust volatile handling of the operand scanner
  2021-08-11  6:40   ` Richard Biener
@ 2021-08-11  8:29     ` Eric Botcazou
  2021-08-11  9:24     ` Richard Biener
  1 sibling, 0 replies; 8+ messages in thread
From: Eric Botcazou @ 2021-08-11  8:29 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, jakub, fortran

> build3 currently does no special processing for the FIELD_DECL operand,
> it just sets TREE_THIS_VOLATILE from operand zero for tcc_references.
> 
> The C and C++ frontends have repeated patterns like
> 
>           ref = build3 (COMPONENT_REF, subtype, datum, subdatum,
>                         NULL_TREE);
>           SET_EXPR_LOCATION (ref, loc);
>           if (TREE_READONLY (subdatum)
> 
>               || (use_datum_quals && TREE_READONLY (datum)))
> 
>             TREE_READONLY (ref) = 1;
>           if (TREE_THIS_VOLATILE (subdatum)
> 
>               || (use_datum_quals && TREE_THIS_VOLATILE (datum)))
> 
>             TREE_THIS_VOLATILE (ref) = 1;

Likewise in the Ada front-end (gigi).

> Now - I wonder if there's a reason a frontend might _not_ want to
> set TREE_THIS_VOLATILE on a COMPONENT_REF when the FIELD_DECL has
> TREE_THIS_VOLATILE set.

This would be weird semantics in my opinion.

> I guess I'll do one more experiment and add verification that
> TREE_THIS_VOLATILE on COMPONENT_REFs and FIELD_DECLs is consistent
> and see where that trips.

Sounds good to me.

-- 
Eric Botcazou




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][v2] Adjust volatile handling of the operand scanner
  2021-08-11  6:40   ` Richard Biener
  2021-08-11  8:29     ` Eric Botcazou
@ 2021-08-11  9:24     ` Richard Biener
  2021-08-11 10:10       ` Eric Botcazou
  1 sibling, 1 reply; 8+ messages in thread
From: Richard Biener @ 2021-08-11  9:24 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: gcc-patches, jakub, fortran

On Wed, 11 Aug 2021, Richard Biener wrote:

> On Tue, 10 Aug 2021, Eric Botcazou wrote:
> 
> > > The question is whether we instead want to amend build3 to
> > > set TREE_THIS_VOLATILE automatically when the FIELD_DECL has
> > > it set.  At least for the Fortran FE cases the gimplifier
> > > fails to see some volatile references and thus can generate
> > > wrong code which is a latent issue.
> > 
> > What do we do for other similar flags, e.g. TREE_READONLY?
> 
> build3 currently does no special processing for the FIELD_DECL operand,
> it just sets TREE_THIS_VOLATILE from operand zero for tcc_references.
> 
> The C and C++ frontends have repeated patterns like
> 
>           ref = build3 (COMPONENT_REF, subtype, datum, subdatum,
>                         NULL_TREE);
>           SET_EXPR_LOCATION (ref, loc);
>           if (TREE_READONLY (subdatum)
>               || (use_datum_quals && TREE_READONLY (datum)))
>             TREE_READONLY (ref) = 1;
>           if (TREE_THIS_VOLATILE (subdatum)
>               || (use_datum_quals && TREE_THIS_VOLATILE (datum)))
>             TREE_THIS_VOLATILE (ref) = 1;
> 
> Leaving out TREE_READONLY shouldn't have any correctness issue.  It's
> just that when adjusting the SSA operand scanner to correctly interpret
> GENERIC that this uncovers pre-existing issues in the Fortran frontend
> (one manifests in a testsuite FAIL - otherwise I wouldn't have noticed).
> 
> I'm fine with just plugging the Fortran FE holes as we discover them
> but I did not check other frontends and testsuite coverage is weak.
> 
> Now - I wonder if there's a reason a frontend might _not_ want to
> set TREE_THIS_VOLATILE on a COMPONENT_REF when the FIELD_DECL has
> TREE_THIS_VOLATILE set.
> 
> I guess I'll do one more experiment and add verification that
> TREE_THIS_VOLATILE on COMPONENT_REFs and FIELD_DECLs is consistent
> and see where that trips.

It trips for

struct X { volatile int i; };

void foo ()
{
  struct X x = (struct X){ .i = 0 };
}

where the gimplifier in gimplify_init_ctor_eval does

          gcc_assert (TREE_CODE (purpose) == FIELD_DECL);
          cref = build3 (COMPONENT_REF, TREE_TYPE (purpose),
                         unshare_expr (object), purpose, NULL_TREE);

producing

  x.i = 0;

that is not volatile qualified.  This manifests itself during the
build of libasan.  I'm not sure whether the gimplifiers doing is
correct or not.  Changing build3 would alter the behavior here.

Then there's a case where the COMPONENT_REF is TREE_THIS_VOLATILE
but neither the FIELD_DECL nor the base reference is.  This
trips during libtsan build and again is from gimplification/folding,
this time gimplify_modify_expr_rhs doing

        case INDIRECT_REF:
          {
            /* If we have code like

             *(const A*)(A*)&x

             where the type of "x" is a (possibly cv-qualified variant
             of "A"), treat the entire expression as identical to "x".
             This kind of code arises in C++ when an object is bound
             to a const reference, and if "x" is a TARGET_EXPR we want
             to take advantage of the optimization below.  */
            bool volatile_p = TREE_THIS_VOLATILE (*from_p);
            tree t = gimple_fold_indirect_ref_rhs (TREE_OPERAND (*from_p, 
0));
            if (t)
              {
                if (TREE_THIS_VOLATILE (t) != volatile_p)
                  {
                    if (DECL_P (t))
                      t = build_simple_mem_ref_loc (EXPR_LOCATION 
(*from_p),
                                                    build_fold_addr_expr 
(t));
                    if (REFERENCE_CLASS_P (t))
                      TREE_THIS_VOLATILE (t) = volatile_p;

I suppose that's OK, it's folding volatile
*(void (*__sanitizer_sighandler_ptr) (int) *) &act->D.5368.handler
to act->D.5368.handler which wouldn't be volatile.

The opposite could happen, too, of course - casting away volatileness
for an access but letting that slip through verification would make
it moot.  So ...

With those cases fixed bootstrap runs through and testing reveals
no additional issues apart from the already known
gfortran.dg/volatile11.f90

So I'm leaning towards leaving build3 alone and fixing up frontends
as issues pop up.

Ricahrd.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][v2] Adjust volatile handling of the operand scanner
  2021-08-11  9:24     ` Richard Biener
@ 2021-08-11 10:10       ` Eric Botcazou
  2021-08-11 14:23         ` Richard Biener
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2021-08-11 10:10 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, jakub, fortran

> So I'm leaning towards leaving build3 alone and fixing up frontends
> as issues pop up.

FWIW fine with me.

-- 
Eric Botcazou



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][v2] Adjust volatile handling of the operand scanner
  2021-08-11 10:10       ` Eric Botcazou
@ 2021-08-11 14:23         ` Richard Biener
  0 siblings, 0 replies; 8+ messages in thread
From: Richard Biener @ 2021-08-11 14:23 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: gcc-patches, jakub, fortran

On Wed, 11 Aug 2021, Eric Botcazou wrote:

> > So I'm leaning towards leaving build3 alone and fixing up frontends
> > as issues pop up.
> 
> FWIW fine with me.

OK, so I pushed the original change (reposted below).

Bootstrapped / tested on x86_64-unknown-linux-gnu.

Richard.

From e5a23d54d189f3d160c82f770683288a15c3645e Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Mon, 9 Aug 2021 13:12:08 +0200
Subject: [PATCH] Adjust volatile handling of the operand scanner
To: gcc-patches@gcc.gnu.org

The GIMPLE SSA operand scanner handles COMPONENT_REFs that are
not marked TREE_THIS_VOLATILE but have a TREE_THIS_VOLATILE
FIELD_DECL as volatile.  That's inconsistent in how TREE_THIS_VOLATILE
testing on GENERIC refs works which requires operand zero of
component references to mirror TREE_THIS_VOLATILE to the ref
so that testing TREE_THIS_VOLATILE on the outermost reference
is enough to determine the volatileness.

The following patch thus removes FIELD_DECL scanning from
the GIMPLE SSA operand scanner, possibly leaving fewer stmts
marked as gimple_has_volatile_ops.

It shows we miss at least one case in the fortran frontend, though
there's a suspicious amount of COMPONENT_REF creation compared
to little setting of TREE_THIS_VOLATILE.  This fixes the FAIL
of gfortran.dg/volatile11.f90 that would otherwise occur.

Visually inspecting fortran/ reveals a bunch of likely to fix
cases but I don't know the constraints of 'volatile' uses in
the fortran language to assess whether some of these are not
necessary.

2021-08-09  Richard Biener  <rguenther@suse.de>

gcc/
	* tree-ssa-operands.c (operands_scanner::get_expr_operands):
	Do not look at COMPONENT_REF FIELD_DECLs TREE_THIS_VOLATILE
	to determine has_volatile_ops.

gcc/fortran/
	* trans-common.c (create_common): Set TREE_THIS_VOLATILE on the
	COMPONENT_REF if the field is volatile.
---
 gcc/fortran/trans-common.c | 9 +++++----
 gcc/tree-ssa-operands.c    | 7 +------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/gcc/fortran/trans-common.c b/gcc/fortran/trans-common.c
index a11cf4c839e..7bcf18dc475 100644
--- a/gcc/fortran/trans-common.c
+++ b/gcc/fortran/trans-common.c
@@ -759,10 +759,11 @@ create_common (gfc_common_head *com, segment_info *head, bool saw_equiv)
       else
 	gfc_add_decl_to_function (var_decl);
 
-      SET_DECL_VALUE_EXPR (var_decl,
-			   fold_build3_loc (input_location, COMPONENT_REF,
-					    TREE_TYPE (s->field),
-					    decl, s->field, NULL_TREE));
+      tree comp = build3_loc (input_location, COMPONENT_REF,
+			      TREE_TYPE (s->field), decl, s->field, NULL_TREE);
+      if (TREE_THIS_VOLATILE (s->field))
+	TREE_THIS_VOLATILE (comp) = 1;
+      SET_DECL_VALUE_EXPR (var_decl, comp);
       DECL_HAS_VALUE_EXPR_P (var_decl) = 1;
       GFC_DECL_COMMON_OR_EQUIV (var_decl) = 1;
 
diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
index c15575416dd..ebf7eea3b04 100644
--- a/gcc/tree-ssa-operands.c
+++ b/gcc/tree-ssa-operands.c
@@ -834,12 +834,7 @@ operands_scanner::get_expr_operands (tree *expr_p, int flags)
 	get_expr_operands (&TREE_OPERAND (expr, 0), flags);
 
 	if (code == COMPONENT_REF)
-	  {
-	    if (!(flags & opf_no_vops)
-		&& TREE_THIS_VOLATILE (TREE_OPERAND (expr, 1)))
-	      gimple_set_has_volatile_ops (stmt, true);
-	    get_expr_operands (&TREE_OPERAND (expr, 2), uflags);
-	  }
+	  get_expr_operands (&TREE_OPERAND (expr, 2), uflags);
 	else if (code == ARRAY_REF || code == ARRAY_RANGE_REF)
 	  {
 	    get_expr_operands (&TREE_OPERAND (expr, 1), uflags);
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-08-11 14:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 11:40 [PATCH][v2] Adjust volatile handling of the operand scanner Richard Biener
2021-08-10 15:55 ` Richard Biener
2021-08-10 21:03 ` Eric Botcazou
2021-08-11  6:40   ` Richard Biener
2021-08-11  8:29     ` Eric Botcazou
2021-08-11  9:24     ` Richard Biener
2021-08-11 10:10       ` Eric Botcazou
2021-08-11 14:23         ` Richard Biener

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).