diff mbox series

aarch64: Fix pure/const function attributes for intrinsics

Message ID AS8PR08MB66781F579B4CE1794C246CB3F4BA9@AS8PR08MB6678.eurprd08.prod.outlook.com
State New
Headers show
Series aarch64: Fix pure/const function attributes for intrinsics | expand

Commit Message

Andrew Carlotti June 30, 2022, 4:03 p.m. UTC 
No testcase for this, since I haven't found a way to turn the incorrect
attribute into incorrect codegen.

Bootstrapped and tested on aarch64-none-linux gnu.

gcc/

	* config/aarch64/aarch64-builtins.c
	(aarch64_get_attributes): Fix choice of pure/const attributes.

---

Comments

Richard Biener July 1, 2022, 6:42 a.m. UTC | #1 
On Thu, Jun 30, 2022 at 6:04 PM Andrew Carlotti via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> No testcase for this, since I haven't found a way to turn the incorrect
> attribute into incorrect codegen.
>
> Bootstrapped and tested on aarch64-none-linux gnu.
>
> gcc/
>
>         * config/aarch64/aarch64-builtins.c
>         (aarch64_get_attributes): Fix choice of pure/const attributes.
>
> ---
>
> diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
> index e0a741ac663188713e21f457affa57217d074783..877f54aab787862794413259cd36ca0fb7bd49c5 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -1085,9 +1085,9 @@ aarch64_get_attributes (unsigned int f, machine_mode mode)
>    if (!aarch64_modifies_global_state_p (f, mode))
>      {
>        if (aarch64_reads_global_state_p (f, mode))
> -       attrs = aarch64_add_attribute ("pure", attrs);
> -      else
>         attrs = aarch64_add_attribute ("const", attrs);
> +      else
> +       attrs = aarch64_add_attribute ("pure", attrs);

that looks backwards.  'pure' allows read of global memory while
'const' does not.  Is
aarch64_reads_global_state_p really backwards?

>      }
>
>    if (!flag_non_call_exceptions || !aarch64_could_trap_p (f, mode))
Andrew Carlotti July 1, 2022, 3:59 p.m. UTC | #2 
On Fri, Jul 01, 2022 at 08:42:15AM +0200, Richard Biener wrote:
> On Thu, Jun 30, 2022 at 6:04 PM Andrew Carlotti via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> > diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
> > index e0a741ac663188713e21f457affa57217d074783..877f54aab787862794413259cd36ca0fb7bd49c5 100644
> > --- a/gcc/config/aarch64/aarch64-builtins.cc
> > +++ b/gcc/config/aarch64/aarch64-builtins.cc
> > @@ -1085,9 +1085,9 @@ aarch64_get_attributes (unsigned int f, machine_mode mode)
> >    if (!aarch64_modifies_global_state_p (f, mode))
> >      {
> >        if (aarch64_reads_global_state_p (f, mode))
> > -       attrs = aarch64_add_attribute ("pure", attrs);
> > -      else
> >         attrs = aarch64_add_attribute ("const", attrs);
> > +      else
> > +       attrs = aarch64_add_attribute ("pure", attrs);
> 
> that looks backwards.  'pure' allows read of global memory while
> 'const' does not.  Is
> aarch64_reads_global_state_p really backwards?

Oh - the thing that's backwards is my understanding of what "pure" and
"const" mean. Their meanings as GCC function attributes seem to be
approximately the opposite way round to their meanings in general usage.
Richard Biener July 4, 2022, 6:49 a.m. UTC | #3 
On Fri, Jul 1, 2022 at 5:59 PM Andrew Carlotti <andrew.carlotti@arm.com> wrote:
>
> On Fri, Jul 01, 2022 at 08:42:15AM +0200, Richard Biener wrote:
> > On Thu, Jun 30, 2022 at 6:04 PM Andrew Carlotti via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > > diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
> > > index e0a741ac663188713e21f457affa57217d074783..877f54aab787862794413259cd36ca0fb7bd49c5 100644
> > > --- a/gcc/config/aarch64/aarch64-builtins.cc
> > > +++ b/gcc/config/aarch64/aarch64-builtins.cc
> > > @@ -1085,9 +1085,9 @@ aarch64_get_attributes (unsigned int f, machine_mode mode)
> > >    if (!aarch64_modifies_global_state_p (f, mode))
> > >      {
> > >        if (aarch64_reads_global_state_p (f, mode))
> > > -       attrs = aarch64_add_attribute ("pure", attrs);
> > > -      else
> > >         attrs = aarch64_add_attribute ("const", attrs);
> > > +      else
> > > +       attrs = aarch64_add_attribute ("pure", attrs);
> >
> > that looks backwards.  'pure' allows read of global memory while
> > 'const' does not.  Is
> > aarch64_reads_global_state_p really backwards?
>
> Oh - the thing that's backwards is my understanding of what "pure" and
> "const" mean. Their meanings as GCC function attributes seem to be
> approximately the opposite way round to their meanings in general usage.

I bet GCCs reading is older ;)

Richard.
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
index e0a741ac663188713e21f457affa57217d074783..877f54aab787862794413259cd36ca0fb7bd49c5 100644
--- a/gcc/config/aarch64/aarch64-builtins.cc
+++ b/gcc/config/aarch64/aarch64-builtins.cc
@@ -1085,9 +1085,9 @@  aarch64_get_attributes (unsigned int f, machine_mode mode)
   if (!aarch64_modifies_global_state_p (f, mode))
     {
       if (aarch64_reads_global_state_p (f, mode))
-       attrs = aarch64_add_attribute ("pure", attrs);
-      else
        attrs = aarch64_add_attribute ("const", attrs);
+      else
+       attrs = aarch64_add_attribute ("pure", attrs);
     }

   if (!flag_non_call_exceptions || !aarch64_could_trap_p (f, mode))