[PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

Hi All,

This patch makes the feature detection code for AArch64 GCC not add features
automatically when the feature had no hwcaps string to match against.

This means that -mcpu=native no longer adds feature flags such as +profile.
The behavior wasn't noticed before because at the time +profile was added a bug
was preventing any feature bits from being added by native detections.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2018-12-18  Tamar Christina  <tamar.christina@arm.com>

	PR target/88530
	* config/aarch64/aarch64-option-extensions.def: Document it.
	* config/aarch64/driver-aarch64.c (host_detect_local_cpu): Skip feature
	if empty hwcaps.

gcc/testsuite/ChangeLog:

2018-12-18  Tamar Christina  <tamar.christina@arm.com>

	PR target/88530
	* gcc.target/aarch64/options_set_10.c: New test.

-- 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rb10438.patch --]
[-- Type: text/x-diff; name="rb10438.patch", Size: 2211 bytes --]

diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index cdf04e2d5fcccb8b9a32af8f83501ce23212bbab..323e642af2e87c2d463681c3a3efbaeff2ede018 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -43,7 +43,8 @@
    the extension (for example, the 'crypto' extension depends on four
    entries: aes, pmull, sha1, sha2 being present).  In that case this field
    should contain a space (" ") separated list of the strings in 'Features'
-   that are required.  Their order is not important.  */
+   that are required.  Their order is not important.  An empty string means
+   do not detect this feature during auto detection.  */
 
 /* Enabling "fp" just enables "fp".
    Disabling "fp" also disables "simd", "crypto", "fp16", "aes", "sha2",
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 98f9d7959506338bd6a8524500a168cc22ef5396..4f386dbf5fc29cc54ff85e062d0b9cd146fa00e8 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -253,6 +253,12 @@ host_detect_local_cpu (int argc, const char **argv)
 	      char *p = NULL;
 	      char *feat_string
 		= concat (aarch64_extensions[i].feat_string, NULL);
+
+	      /* If the feature contains no HWCAPS string then ignore it for the
+		 auto detection.  */
+	      if (strlen (feat_string) == 0)
+		continue;
+
 	      bool enabled = true;
 
 	      /* This may be a multi-token feature string.  We need
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_10.c b/gcc/testsuite/gcc.target/aarch64/options_set_10.c
new file mode 100644
index 0000000000000000000000000000000000000000..5ffe83c199165dd4129814674297056bdf27cd83
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_10.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target "aarch64*-*-linux*" } } */
+/* { dg-additional-options "-mcpu=native" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not {\.arch .+\+profile.*} } } */
+
+ /* Check that an empty feature string is not detected during mcpu=native.  */

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Kyrill Tkachov]

Hi Tamar,

On 18/12/18 13:36, Tamar Christina wrote:
> Hi All,
>
> This patch makes the feature detection code for AArch64 GCC not add features
> automatically when the feature had no hwcaps string to match against.
>
> This means that -mcpu=native no longer adds feature flags such as +profile.
> The behavior wasn't noticed before because at the time +profile was added a bug
> was preventing any feature bits from being added by native detections.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for trunk?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 2018-12-18  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/88530
>         * config/aarch64/aarch64-option-extensions.def: Document it.
>         * config/aarch64/driver-aarch64.c (host_detect_local_cpu): Skip feature
>         if empty hwcaps.
>
> gcc/testsuite/ChangeLog:
>
> 2018-12-18  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/88530
>         * gcc.target/aarch64/options_set_10.c: New test.
>
> -- 

diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index cdf04e2d5fcccb8b9a32af8f83501ce23212bbab..323e642af2e87c2d463681c3a3efbaeff2ede018 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -43,7 +43,8 @@
     the extension (for example, the 'crypto' extension depends on four
     entries: aes, pmull, sha1, sha2 being present).  In that case this field
     should contain a space (" ") separated list of the strings in 'Features'
-   that are required.  Their order is not important.  */
+   that are required.  Their order is not important.  An empty string means
+   do not detect this feature during auto detection.  */
  
  /* Enabling "fp" just enables "fp".
     Disabling "fp" also disables "simd", "crypto", "fp16", "aes", "sha2",
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 98f9d7959506338bd6a8524500a168cc22ef5396..4f386dbf5fc29cc54ff85e062d0b9cd146fa00e8 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -253,6 +253,12 @@ host_detect_local_cpu (int argc, const char **argv)
  	      char *p = NULL;
  	      char *feat_string
  		= concat (aarch64_extensions[i].feat_string, NULL);
+
+	      /* If the feature contains no HWCAPS string then ignore it for the
+		 auto detection.  */
+	      if (strlen (feat_string) == 0)
+		continue;

I think this can avoid a strlen call by checking (*feat_string == '\0') though I believe most compilers will optimise it that way anyway.
It might be more immediately readable your way.
I wouldn't let it hold off this patch.

Looks good to me, but you'll need a maintainer to approve.

Thanks,
Kyrill


+
  	      bool enabled = true;
  
  	      /* This may be a multi-token feature string.  We need
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_10.c b/gcc/testsuite/gcc.target/aarch64/options_set_10.c
new file mode 100644
index 0000000000000000000000000000000000000000..5ffe83c199165dd4129814674297056bdf27cd83
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_10.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target "aarch64*-*-linux*" } } */
+/* { dg-additional-options "-mcpu=native" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not {\.arch .+\+profile.*} } } */
+
+ /* Check that an empty feature string is not detected during mcpu=native.  */

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Jakub Jelinek]

On Thu, Jan 10, 2019 at 04:57:47PM +0000, Kyrill Tkachov wrote:
> --- a/gcc/config/aarch64/driver-aarch64.c
> +++ b/gcc/config/aarch64/driver-aarch64.c
> @@ -253,6 +253,12 @@ host_detect_local_cpu (int argc, const char **argv)
>  	      char *p = NULL;
>  	      char *feat_string
>  		= concat (aarch64_extensions[i].feat_string, NULL);
> +
> +	      /* If the feature contains no HWCAPS string then ignore it for the
> +		 auto detection.  */
> +	      if (strlen (feat_string) == 0)
> +		continue;
> 
> I think this can avoid a strlen call by checking (*feat_string == '\0') though I believe most compilers will optimise it that way anyway.
> It might be more immediately readable your way.
> I wouldn't let it hold off this patch.

Well, that isn't the only problem of this code.
Another one is that concat (str, NULL) is much better written as xstrdup (str);
Another one is that the if (*feat_string == '\0') check
should be probably if (aarch64_extensions[i].feat_string[0] == '\0')
before the xstrdup, because there is no point to allocate the memory
in that case.
Another one is that it leaks the feat_string (right now always, otherwise
if it isn't empty).

Another one, I wonder if the xstrdup + strtok isn't a too heavy hammer for
something so simple, especially when you have complete control over those
feature strings.  They use exactly one space to separate.
So just do
	      const char *p = aarch64_extensions[i].feat_string;
	      bool enabled = true;

	      /* If the feature contains no HWCAPS string then ignore it for the
		 auto detection.  */
	      if (*p == '\0')
		continue;

	      size_t len = strlen (buf);
	      do
		{
		  const char *end = strchr (p, ' ');
		  if (end == NULL)
		    end = strchr (p, '\0');
		  if (memmem (buf, len, p, end - p) == NULL)
		    {
		      /* Failed to match this token.  Turn off the
			 features we'd otherwise enable.  */
		      enabled = false;
		      break;
		    }
		  if (*end == '\0')
		    break;
		  p = end + 1;
		}
	      while (1);

	      if (enabled)
		extension_flags |= aarch64_extensions[i].flag;
	      else
		extension_flags &= ~(aarch64_extensions[i].flag);
?

Last thing, for not_found, there is:
return "";, why not return NULL; ?  That is what other detect cpu routines
do, returning "" means a confusion between when a heap allocated string is
returned that needs freeing and when a .rodata string is returned which
doesn't.

	Jakub

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

Hi Jakub,

Thanks for the feedback, but I think those are changes for another patch.
The patch here does not introduce the problems you highlighted and I don't feel these changes are appropriate for stage4 or that they should block this patch.

Kind regards,
Tamar


________________________________________
From: Jakub Jelinek <jakub@redhat.com>
Sent: Wednesday, January 23, 2019 4:27:40 PM
To: Kyrill Tkachov
Cc: Tamar Christina; gcc-patches@gcc.gnu.org; nd; James Greenhalgh; Richard Earnshaw; Marcus Shawcroft
Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

On Thu, Jan 10, 2019 at 04:57:47PM +0000, Kyrill Tkachov wrote:
> --- a/gcc/config/aarch64/driver-aarch64.c
> +++ b/gcc/config/aarch64/driver-aarch64.c
> @@ -253,6 +253,12 @@ host_detect_local_cpu (int argc, const char **argv)
>             char *p = NULL;
>             char *feat_string
>               = concat (aarch64_extensions[i].feat_string, NULL);
> +
> +           /* If the feature contains no HWCAPS string then ignore it for the
> +              auto detection.  */
> +           if (strlen (feat_string) == 0)
> +             continue;
>
> I think this can avoid a strlen call by checking (*feat_string == '\0') though I believe most compilers will optimise it that way anyway.
> It might be more immediately readable your way.
> I wouldn't let it hold off this patch.

Well, that isn't the only problem of this code.
Another one is that concat (str, NULL) is much better written as xstrdup (str);
Another one is that the if (*feat_string == '\0') check
should be probably if (aarch64_extensions[i].feat_string[0] == '\0')
before the xstrdup, because there is no point to allocate the memory
in that case.
Another one is that it leaks the feat_string (right now always, otherwise
if it isn't empty).

Another one, I wonder if the xstrdup + strtok isn't a too heavy hammer for
something so simple, especially when you have complete control over those
feature strings.  They use exactly one space to separate.
So just do
              const char *p = aarch64_extensions[i].feat_string;
              bool enabled = true;

              /* If the feature contains no HWCAPS string then ignore it for the
                 auto detection.  */
              if (*p == '\0')
                continue;

              size_t len = strlen (buf);
              do
                {
                  const char *end = strchr (p, ' ');
                  if (end == NULL)
                    end = strchr (p, '\0');
                  if (memmem (buf, len, p, end - p) == NULL)
                    {
                      /* Failed to match this token.  Turn off the
                         features we'd otherwise enable.  */
                      enabled = false;
                      break;
                    }
                  if (*end == '\0')
                    break;
                  p = end + 1;
                }
              while (1);

              if (enabled)
                extension_flags |= aarch64_extensions[i].flag;
              else
                extension_flags &= ~(aarch64_extensions[i].flag);
?

Last thing, for not_found, there is:
return "";, why not return NULL; ?  That is what other detect cpu routines
do, returning "" means a confusion between when a heap allocated string is
returned that needs freeing and when a .rodata string is returned which
doesn't.

        Jakub

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Jakub Jelinek]

On Wed, Jan 30, 2019 at 02:06:01PM +0000, Tamar Christina wrote:
> Thanks for the feedback, but I think those are changes for another patch.

At least the memory leak is something that should be fixed even in stage4
IMNSHO.

Anyway, will defer to aarch64 maintainers here.

Just one question, for the *feat_string == '\0' case, is continue what you
want, rather than just enabled = false; and doing the
  extension_flags &= ~(aarch64_extensions[i].flag);
later on?

	Jakub

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

Hi Jakub,

On Wed, Jan 30, 2019 at 02:06:01PM +0000, Tamar Christina wrote:
> > Thanks for the feedback, but I think those are changes for another patch.
>
> At least the memory leak is something that should be fixed even in stage4
> IMNSHO.

I'll provide a separate patch for this then.

> Anyway, will defer to aarch64 maintainers here.

> Just one question, for the *feat_string == '\0' case, is continue what you
> want, rather than just enabled = false; and doing the
 > extension_flags &= ~(aarch64_extensions[i].flag);
> later on?

Yeah, because the feature may be on by default due to another extension, in
which case you would erroneously turn it off. The absence of an HWCAPS
shouldn't pro-actively disable an extension.

Regards,
Tamar

RE: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

[-- Attachment #1: Type: text/plain, Size: 2554 bytes --]

Hi All,

Since this hasn't been reviewed yet anyway I've updated this patch to also fix the memory leaks etc.

--

This patch makes the feature detection code for AArch64 GCC not add features
automatically when the feature had no hwcaps string to match against.

This means that -mcpu=native no longer adds feature flags such as +profile.
The behavior wasn't noticed before because at the time +profile was added a bug
was preventing any feature bits from being added by native detections.

The loop has also been changed as Jakub specified in order to avoid a memory
leak that was present in the existing code and to be slightly more efficient.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-02-07  Tamar Christina  <tamar.christina@arm.com>

	PR target/88530
	* config/aarch64/aarch64-option-extensions.def: Document it.
	* config/aarch64/driver-aarch64.c (host_detect_local_cpu): Skip feature
	if empty hwcaps.

gcc/testsuite/ChangeLog:

2019-02-07  Tamar Christina  <tamar.christina@arm.com>

	PR target/88530
	* gcc.target/aarch64/options_set_10.c: New test.

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org>
> On Behalf Of Tamar Christina
> Sent: Wednesday, January 30, 2019 14:48
> To: Jakub Jelinek <jakub@redhat.com>
> Cc: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>; gcc-patches@gcc.gnu.org;
> nd <nd@arm.com>; James Greenhalgh <James.Greenhalgh@arm.com>;
> Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>
> Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> during native feature detection
> 
> Hi Jakub,
> 
> On Wed, Jan 30, 2019 at 02:06:01PM +0000, Tamar Christina wrote:
> > > Thanks for the feedback, but I think those are changes for another patch.
> >
> > At least the memory leak is something that should be fixed even in
> > stage4 IMNSHO.
> 
> I'll provide a separate patch for this then.
> 
> > Anyway, will defer to aarch64 maintainers here.
> 
> > Just one question, for the *feat_string == '\0' case, is continue what
> > you want, rather than just enabled = false; and doing the
>  > extension_flags &= ~(aarch64_extensions[i].flag);
> > later on?
> 
> Yeah, because the feature may be on by default due to another extension, in
> which case you would erroneously turn it off. The absence of an HWCAPS
> shouldn't pro-actively disable an extension.
> 
> Regards,
> Tamar

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rb10438.patch --]
[-- Type: text/x-diff; name="rb10438.patch", Size: 3501 bytes --]

diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index 50909debf5455d57b86db91a0a5539fed1d5b91e..a6b3ae2b73f285e62e0f24c92bc5efefc9ffb59c 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -43,7 +43,8 @@
    the extension (for example, the 'crypto' extension depends on four
    entries: aes, pmull, sha1, sha2 being present).  In that case this field
    should contain a space (" ") separated list of the strings in 'Features'
-   that are required.  Their order is not important.  */
+   that are required.  Their order is not important.  An empty string means
+   do not detect this feature during auto detection.  */
 
 /* Enabling "fp" just enables "fp".
    Disabling "fp" also disables "simd", "crypto", "fp16", "aes", "sha2",
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 05f1360d48583473820c8008cc09ed139bddc0ce..9515061ec985cc374c7c510c6954a6a3c240814f 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -250,27 +250,35 @@ host_detect_local_cpu (int argc, const char **argv)
 	{
 	  for (i = 0; i < num_exts; i++)
 	    {
-	      char *p = NULL;
-	      char *feat_string
-		= concat (aarch64_extensions[i].feat_string, NULL);
+	      const char *p = aarch64_extensions[i].feat_string;
+
+	      /* If the feature contains no HWCAPS string then ignore it for the
+		 auto detection.  */
+	      if (*p == '\0')
+		continue;
+
 	      bool enabled = true;
 
 	      /* This may be a multi-token feature string.  We need
-		 to match all parts, which could be in any order.
-		 If this isn't a multi-token feature string, strtok is
-		 just going to return a pointer to feat_string.  */
-	      p = strtok (feat_string, " ");
-	      while (p != NULL)
+		 to match all parts, which could be in any order.  */
+	      size_t len = strlen (buf);
+	      do
 		{
-		  if (strstr (buf, p) == NULL)
+		  const char *end = strchr (p, ' ');
+		  if (end == NULL)
+		    end = strchr (p, '\0');
+		  if (memmem (buf, len, p, end - p) == NULL)
 		    {
 		      /* Failed to match this token.  Turn off the
 			 features we'd otherwise enable.  */
 		      enabled = false;
 		      break;
 		    }
-		  p = strtok (NULL, " ");
+		  if (*end == '\0')
+		    break;
+		  p = end + 1;
 		}
+	      while (1);
 
 	      if (enabled)
 		extension_flags |= aarch64_extensions[i].flag;
@@ -360,12 +368,12 @@ host_detect_local_cpu (int argc, const char **argv)
 not_found:
   {
    /* If detection fails we ignore the option.
-      Clean up and return empty string.  */
+      Clean up and return NULL.  */
 
     if (f)
       fclose (f);
 
-    return "";
+    return NULL;
   }
 }
 
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_10.c b/gcc/testsuite/gcc.target/aarch64/options_set_10.c
new file mode 100644
index 0000000000000000000000000000000000000000..5ffe83c199165dd4129814674297056bdf27cd83
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_10.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target "aarch64*-*-linux*" } } */
+/* { dg-additional-options "-mcpu=native" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not {\.arch .+\+profile.*} } } */
+
+ /* Check that an empty feature string is not detected during mcpu=native.  */

RE: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

Ping.

> -----Original Message-----
> From: Tamar Christina <Tamar.Christina@arm.com>
> Sent: Thursday, February 7, 2019 10:43
> To: Tamar Christina <Tamar.Christina@arm.com>; Jakub Jelinek
> <jakub@redhat.com>
> Cc: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>; gcc-patches@gcc.gnu.org;
> nd <nd@arm.com>; James Greenhalgh <James.Greenhalgh@arm.com>;
> Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>
> Subject: RE: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> during native feature detection
> 
> Hi All,
> 
> Since this hasn't been reviewed yet anyway I've updated this patch to also fix
> the memory leaks etc.
> 
> --
> 
> This patch makes the feature detection code for AArch64 GCC not add
> features automatically when the feature had no hwcaps string to match
> against.
> 
> This means that -mcpu=native no longer adds feature flags such as +profile.
> The behavior wasn't noticed before because at the time +profile was added
> a bug was preventing any feature bits from being added by native detections.
> 
> The loop has also been changed as Jakub specified in order to avoid a
> memory leak that was present in the existing code and to be slightly more
> efficient.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for trunk?
> 
> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/88530
> 	* config/aarch64/aarch64-option-extensions.def: Document it.
> 	* config/aarch64/driver-aarch64.c (host_detect_local_cpu): Skip
> feature
> 	if empty hwcaps.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/88530
> 	* gcc.target/aarch64/options_set_10.c: New test.
> 
> > -----Original Message-----
> > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org>
> On
> > Behalf Of Tamar Christina
> > Sent: Wednesday, January 30, 2019 14:48
> > To: Jakub Jelinek <jakub@redhat.com>
> > Cc: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>;
> > gcc-patches@gcc.gnu.org; nd <nd@arm.com>; James Greenhalgh
> > <James.Greenhalgh@arm.com>; Richard Earnshaw
> > <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> > <Marcus.Shawcroft@arm.com>
> > Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> > during native feature detection
> >
> > Hi Jakub,
> >
> > On Wed, Jan 30, 2019 at 02:06:01PM +0000, Tamar Christina wrote:
> > > > Thanks for the feedback, but I think those are changes for another
> patch.
> > >
> > > At least the memory leak is something that should be fixed even in
> > > stage4 IMNSHO.
> >
> > I'll provide a separate patch for this then.
> >
> > > Anyway, will defer to aarch64 maintainers here.
> >
> > > Just one question, for the *feat_string == '\0' case, is continue
> > > what you want, rather than just enabled = false; and doing the
> >  > extension_flags &= ~(aarch64_extensions[i].flag);
> > > later on?
> >
> > Yeah, because the feature may be on by default due to another
> > extension, in which case you would erroneously turn it off. The
> > absence of an HWCAPS shouldn't pro-actively disable an extension.
> >
> > Regards,
> > Tamar

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[James Greenhalgh]

On Thu, Feb 07, 2019 at 04:43:24AM -0600, Tamar Christina wrote:
> Hi All,
> 
> Since this hasn't been reviewed yet anyway I've updated this patch to also fix the memory leaks etc.
> 
> --
> 
> This patch makes the feature detection code for AArch64 GCC not add features
> automatically when the feature had no hwcaps string to match against.
> 
> This means that -mcpu=native no longer adds feature flags such as +profile.
> The behavior wasn't noticed before because at the time +profile was added a bug
> was preventing any feature bits from being added by native detections.
> 
> The loop has also been changed as Jakub specified in order to avoid a memory
> leak that was present in the existing code and to be slightly more efficient.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for trunk?

OK. Is this also desirable for a backport?

Thanks,
James

> 
> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/88530
> 	* config/aarch64/aarch64-option-extensions.def: Document it.
> 	* config/aarch64/driver-aarch64.c (host_detect_local_cpu): Skip feature
> 	if empty hwcaps.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/88530
> 	* gcc.target/aarch64/options_set_10.c: New test.
> 

RE: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

Hi James,

> -----Original Message-----
> From: James Greenhalgh <james.greenhalgh@arm.com>
> Sent: Wednesday, February 27, 2019 17:22
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: Jakub Jelinek <jakub@redhat.com>; Kyrill Tkachov
> <kyrylo.tkachov@foss.arm.com>; gcc-patches@gcc.gnu.org; nd
> <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus
> Shawcroft <Marcus.Shawcroft@arm.com>
> Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> during native feature detection
> 
> On Thu, Feb 07, 2019 at 04:43:24AM -0600, Tamar Christina wrote:
> > Hi All,
> >
> > Since this hasn't been reviewed yet anyway I've updated this patch to also
> fix the memory leaks etc.
> >
> > --
> >
> > This patch makes the feature detection code for AArch64 GCC not add
> > features automatically when the feature had no hwcaps string to match
> against.
> >
> > This means that -mcpu=native no longer adds feature flags such as +profile.
> > The behavior wasn't noticed before because at the time +profile was
> > added a bug was preventing any feature bits from being added by native
> detections.
> >
> > The loop has also been changed as Jakub specified in order to avoid a
> > memory leak that was present in the existing code and to be slightly more
> efficient.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >
> > Ok for trunk?
> 
> OK. Is this also desirable for a backport?

Yes I believe we have this problem in GCC8 as well the profile extensions.

Kind regards,
Tamar

> 
> Thanks,
> James
> 
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> >
> > 	PR target/88530
> > 	* config/aarch64/aarch64-option-extensions.def: Document it.
> > 	* config/aarch64/driver-aarch64.c (host_detect_local_cpu): Skip
> feature
> > 	if empty hwcaps.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> >
> > 	PR target/88530
> > 	* gcc.target/aarch64/options_set_10.c: New test.
> >

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Christophe Lyon]

On Wed, 27 Feb 2019 at 18:32, Tamar Christina <Tamar.Christina@arm.com> wrote:
>
> Hi James,
>
> > -----Original Message-----
> > From: James Greenhalgh <james.greenhalgh@arm.com>
> > Sent: Wednesday, February 27, 2019 17:22
> > To: Tamar Christina <Tamar.Christina@arm.com>
> > Cc: Jakub Jelinek <jakub@redhat.com>; Kyrill Tkachov
> > <kyrylo.tkachov@foss.arm.com>; gcc-patches@gcc.gnu.org; nd
> > <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus
> > Shawcroft <Marcus.Shawcroft@arm.com>
> > Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> > during native feature detection
> >
> > On Thu, Feb 07, 2019 at 04:43:24AM -0600, Tamar Christina wrote:
> > > Hi All,
> > >
> > > Since this hasn't been reviewed yet anyway I've updated this patch to also
> > fix the memory leaks etc.
> > >
> > > --
> > >
> > > This patch makes the feature detection code for AArch64 GCC not add
> > > features automatically when the feature had no hwcaps string to match
> > against.
> > >
> > > This means that -mcpu=native no longer adds feature flags such as +profile.
> > > The behavior wasn't noticed before because at the time +profile was
> > > added a bug was preventing any feature bits from being added by native
> > detections.
> > >
> > > The loop has also been changed as Jakub specified in order to avoid a
> > > memory leak that was present in the existing code and to be slightly more
> > efficient.
> > >
> > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > >
> > > Ok for trunk?
> >
> > OK. Is this also desirable for a backport?
>
> Yes I believe we have this problem in GCC8 as well the profile extensions.
>
> Kind regards,
> Tamar
>

Hi Tamar,

The new test fails with a cross-compiler, because:
FAIL: gcc.target/aarch64/options_set_10.c (test for excess errors)
Excess errors:
cc1: error: unknown value 'native' for -mcpu

I don't know how to restrict tests to native compilers only.

Christophe

> >
> > Thanks,
> > James
> >
> > >
> > > Thanks,
> > > Tamar
> > >
> > > gcc/ChangeLog:
> > >
> > > 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> > >
> > >     PR target/88530
> > >     * config/aarch64/aarch64-option-extensions.def: Document it.
> > >     * config/aarch64/driver-aarch64.c (host_detect_local_cpu): Skip
> > feature
> > >     if empty hwcaps.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> > >
> > >     PR target/88530
> > >     * gcc.target/aarch64/options_set_10.c: New test.
> > >

RE: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

Hi Christophe,

> -----Original Message-----
> From: Christophe Lyon <christophe.lyon@linaro.org>
> Sent: Monday, March 4, 2019 13:32
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: James Greenhalgh <James.Greenhalgh@arm.com>; Jakub Jelinek
> <jakub@redhat.com>; Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>; gcc-
> patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw
> <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>
> Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> during native feature detection
> 
> On Wed, 27 Feb 2019 at 18:32, Tamar Christina <Tamar.Christina@arm.com>
> wrote:
> >
> > Hi James,
> >
> > > -----Original Message-----
> > > From: James Greenhalgh <james.greenhalgh@arm.com>
> > > Sent: Wednesday, February 27, 2019 17:22
> > > To: Tamar Christina <Tamar.Christina@arm.com>
> > > Cc: Jakub Jelinek <jakub@redhat.com>; Kyrill Tkachov
> > > <kyrylo.tkachov@foss.arm.com>; gcc-patches@gcc.gnu.org; nd
> > > <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>;
> Marcus
> > > Shawcroft <Marcus.Shawcroft@arm.com>
> > > Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> > > during native feature detection
> > >
> > > On Thu, Feb 07, 2019 at 04:43:24AM -0600, Tamar Christina wrote:
> > > > Hi All,
> > > >
> > > > Since this hasn't been reviewed yet anyway I've updated this patch
> > > > to also
> > > fix the memory leaks etc.
> > > >
> > > > --
> > > >
> > > > This patch makes the feature detection code for AArch64 GCC not
> > > > add features automatically when the feature had no hwcaps string
> > > > to match
> > > against.
> > > >
> > > > This means that -mcpu=native no longer adds feature flags such as
> +profile.
> > > > The behavior wasn't noticed before because at the time +profile
> > > > was added a bug was preventing any feature bits from being added
> > > > by native
> > > detections.
> > > >
> > > > The loop has also been changed as Jakub specified in order to
> > > > avoid a memory leak that was present in the existing code and to
> > > > be slightly more
> > > efficient.
> > > >
> > > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > > >
> > > > Ok for trunk?
> > >
> > > OK. Is this also desirable for a backport?
> >
> > Yes I believe we have this problem in GCC8 as well the profile extensions.
> >
> > Kind regards,
> > Tamar
> >
> 
> Hi Tamar,
> 
> The new test fails with a cross-compiler, because:
> FAIL: gcc.target/aarch64/options_set_10.c (test for excess errors) Excess
> errors:
> cc1: error: unknown value 'native' for -mcpu
> 
> I don't know how to restrict tests to native compilers only.

Ah, thanks, I tested only the elf builds cross. I'll fix it up with a new testsuite
directive that checks if -mcpu=native compiles something.

Regards,
Tamar

> 
> Christophe
> 
> > >
> > > Thanks,
> > > James
> > >
> > > >
> > > > Thanks,
> > > > Tamar
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > > 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> > > >
> > > >     PR target/88530
> > > >     * config/aarch64/aarch64-option-extensions.def: Document it.
> > > >     * config/aarch64/driver-aarch64.c (host_detect_local_cpu):
> > > > Skip
> > > feature
> > > >     if empty hwcaps.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > 2019-02-07  Tamar Christina  <tamar.christina@arm.com>
> > > >
> > > >     PR target/88530
> > > >     * gcc.target/aarch64/options_set_10.c: New test.
> > > >

Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Jakub Jelinek]

On Mon, Mar 04, 2019 at 02:31:57PM +0100, Christophe Lyon wrote:
> The new test fails with a cross-compiler, because:
> FAIL: gcc.target/aarch64/options_set_10.c (test for excess errors)
> Excess errors:
> cc1: error: unknown value 'native' for -mcpu
> 
> I don't know how to restrict tests to native compilers only.

{ target native }
perhaps?

	Jakub

RE: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Tamar Christina]

> -----Original Message-----
> From: Jakub Jelinek <jakub@redhat.com>
> Sent: Monday, March 4, 2019 13:38
> To: Christophe Lyon <christophe.lyon@linaro.org>
> Cc: Tamar Christina <Tamar.Christina@arm.com>; James Greenhalgh
> <James.Greenhalgh@arm.com>; Kyrill Tkachov
> <kyrylo.tkachov@foss.arm.com>; gcc-patches@gcc.gnu.org; nd
> <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus
> Shawcroft <Marcus.Shawcroft@arm.com>
> Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored
> during native feature detection
> 
> On Mon, Mar 04, 2019 at 02:31:57PM +0100, Christophe Lyon wrote:
> > The new test fails with a cross-compiler, because:
> > FAIL: gcc.target/aarch64/options_set_10.c (test for excess errors)
> > Excess errors:
> > cc1: error: unknown value 'native' for -mcpu
> >
> > I don't know how to restrict tests to native compilers only.
> 
> { target native }
> perhaps?

Oh, I didn't know about that one, I knew about isnative , I'll give it a try and see. Thanks!

Regards,
Tamar

> 
> 	Jakub