diff mbox series

strub: add note on attribute access

Message ID orbkb0b00e.fsf_-_@lxoliva.fsfla.org
State Unresolved
Headers
Series strub: add note on attribute access |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Alexandre Oliva Dec. 9, 2023, 2:08 a.m. UTC 
  On Dec  7, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> Thanks for raising the issue.  Maybe there should be at least a comment
> there, and perhaps some asserts to check that pointer and reference
> types don't make to indirect_parms.

Document why attribute access doesn't need the same treatment as fn
spec, and check that the assumption behind it holds.

Regstrapped on x86_64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	* ipa-strub.cc (pass_ipa_strub::execute): Check that we don't
	add indirection to pointer parameters, and document attribute
	access non-interactions.
---
 gcc/ipa-strub.cc |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Richard Biener Dec. 11, 2023, 7:26 a.m. UTC | #1 
On Sat, Dec 9, 2023 at 3:09 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Dec  7, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>
> > Thanks for raising the issue.  Maybe there should be at least a comment
> > there, and perhaps some asserts to check that pointer and reference
> > types don't make to indirect_parms.
>
> Document why attribute access doesn't need the same treatment as fn
> spec, and check that the assumption behind it holds.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?

OK

>
> for  gcc/ChangeLog
>
>         * ipa-strub.cc (pass_ipa_strub::execute): Check that we don't
>         add indirection to pointer parameters, and document attribute
>         access non-interactions.
> ---
>  gcc/ipa-strub.cc |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc
> index 2afb7a455751d..8ec6824e8a802 100644
> --- a/gcc/ipa-strub.cc
> +++ b/gcc/ipa-strub.cc
> @@ -2889,6 +2889,13 @@ pass_ipa_strub::execute (function *)
>                 && (tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (nparm)))
>                     <= 4 * UNITS_PER_WORD))))
>         {
> +         /* No point in indirecting pointer types.  Presumably they
> +            won't ever pass the size-based test above, but check the
> +            assumption here, because getting this wrong would mess
> +            with attribute access and possibly others.  We deal with
> +            fn spec below.  */
> +         gcc_checking_assert (!POINTER_TYPE_P (TREE_TYPE (nparm)));
> +
>           indirect_nparms.add (nparm);
>
>           /* ??? Is there any case in which it is not safe to suggest the parms
> @@ -2976,7 +2983,9 @@ pass_ipa_strub::execute (function *)
>                 }
>             }
>
> -       /* ??? Maybe we could adjust it instead.  */
> +       /* ??? Maybe we could adjust it instead.  Note we don't need
> +          to mess with attribute access: pointer-typed parameters are
> +          not modified, so they can remain unchanged.  */
>         if (drop_fnspec)
>           remove_named_attribute_unsharing ("fn spec",
>                                             &TYPE_ATTRIBUTES (nftype));
>
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
Jan Hubicka Dec. 12, 2023, 2:21 p.m. UTC | #2 
> On Dec  7, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
> 
> > Thanks for raising the issue.  Maybe there should be at least a comment
> > there, and perhaps some asserts to check that pointer and reference
> > types don't make to indirect_parms.
> 
> Document why attribute access doesn't need the same treatment as fn
> spec, and check that the assumption behind it holds.
> 
> Regstrapped on x86_64-linux-gnu.  Ok to install?
> 
> 
> for  gcc/ChangeLog
> 
> 	* ipa-strub.cc (pass_ipa_strub::execute): Check that we don't
> 	add indirection to pointer parameters, and document attribute
> 	access non-interactions.
OK,
Honza
diff mbox series

Patch

diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc
index 2afb7a455751d..8ec6824e8a802 100644
--- a/gcc/ipa-strub.cc
+++ b/gcc/ipa-strub.cc
@@ -2889,6 +2889,13 @@  pass_ipa_strub::execute (function *)
 		&& (tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (nparm)))
 		    <= 4 * UNITS_PER_WORD))))
 	{
+	  /* No point in indirecting pointer types.  Presumably they
+	     won't ever pass the size-based test above, but check the
+	     assumption here, because getting this wrong would mess
+	     with attribute access and possibly others.  We deal with
+	     fn spec below.  */
+	  gcc_checking_assert (!POINTER_TYPE_P (TREE_TYPE (nparm)));
+
 	  indirect_nparms.add (nparm);
 
 	  /* ??? Is there any case in which it is not safe to suggest the parms
@@ -2976,7 +2983,9 @@  pass_ipa_strub::execute (function *)
 		}
 	    }
 
-	/* ??? Maybe we could adjust it instead.  */
+	/* ??? Maybe we could adjust it instead.  Note we don't need
+	   to mess with attribute access: pointer-typed parameters are
+	   not modified, so they can remain unchanged.  */
 	if (drop_fnspec)
 	  remove_named_attribute_unsharing ("fn spec",
 					    &TYPE_ATTRIBUTES (nftype));