[AArch64] opt: Fix options canonization for assembler

Message ID 20200402165202.GA32431@arm.com
State New
Headers show
Series
  • [AArch64] opt: Fix options canonization for assembler
Related show

Commit Message

Tamar Christina April 2, 2020, 4:52 p.m.
Hi All,

It is currently impossible to use fp16 on any architecture higher than Armv8.3-a
due to a bug in options canonization.  This bug results in the fp16 flag not
being emitted in the assembly when it should have been.

This is caused by a complicated architectural requirement at Armv8.4-a.  On
Armv8.2-a and Armv8.3-a fp16fml is an optional extension and turning it on turns
on both fp and fp16.  However starting with Armv8.4-a fp16fml is mandatory if
fp16 is available, otherwise it's optional.

In short this means that to enable fp16fml the smallest option that needs to
passed to the assembler is Armv8.4-a+fp16.

The fix in this patch takes into account that an option may be on by default in
an architecture, but that not all the bits required to use it are on by default
in an architecture.  In such cases the difference between the two are still
emitted to the assembler.

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

Ok for trunk? and backport to GCC 8 and 9 after some stew.

Thanks,
Tamar

gcc/ChangeLog:

2020-04-02  Tamar Christina  <tamar.christina@arm.com>

	PR target/94396
	* common/config/aarch64/aarch64-common.c
	(aarch64_get_extension_string_for_isa_flags): Handle default flags.

gcc/testsuite/ChangeLog:

2020-04-02  Tamar Christina  <tamar.christina@arm.com>

	PR target/94396
	* gcc.target/aarch64/options_set_11.c: New test.
	* gcc.target/aarch64/options_set_12.c: New test.
	* gcc.target/aarch64/options_set_13.c: New test.
	* gcc.target/aarch64/options_set_14.c: New test.
	* gcc.target/aarch64/options_set_15.c: New test.
	* gcc.target/aarch64/options_set_16.c: New test.
	* gcc.target/aarch64/options_set_17.c: New test.
	* gcc.target/aarch64/options_set_18.c: New test.
	* gcc.target/aarch64/options_set_19.c: New test.
	* gcc.target/aarch64/options_set_20.c: New test.
	* gcc.target/aarch64/options_set_21.c: New test.
	* gcc.target/aarch64/options_set_22.c: New test.
	* gcc.target/aarch64/options_set_23.c: New test.
	* gcc.target/aarch64/options_set_24.c: New test.
	* gcc.target/aarch64/options_set_25.c: New test.
	* gcc.target/aarch64/options_set_26.c: New test.

--

Comments

Richard Earnshaw (lists) April 3, 2020, 11:32 a.m. | #1
On 02/04/2020 17:52, Tamar Christina wrote:
> Hi All,

> 

> It is currently impossible to use fp16 on any architecture higher than Armv8.3-a

> due to a bug in options canonization.  This bug results in the fp16 flag not

> being emitted in the assembly when it should have been.

> 

> This is caused by a complicated architectural requirement at Armv8.4-a.  On

> Armv8.2-a and Armv8.3-a fp16fml is an optional extension and turning it on turns

> on both fp and fp16.  However starting with Armv8.4-a fp16fml is mandatory if

> fp16 is available, otherwise it's optional.

> 

> In short this means that to enable fp16fml the smallest option that needs to

> passed to the assembler is Armv8.4-a+fp16.

> 

> The fix in this patch takes into account that an option may be on by default in

> an architecture, but that not all the bits required to use it are on by default

> in an architecture.  In such cases the difference between the two are still

> emitted to the assembler.

> 

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

> 

> Ok for trunk? and backport to GCC 8 and 9 after some stew.

> 

> Thanks,

> Tamar

> 

> gcc/ChangeLog:

> 

> 2020-04-02  Tamar Christina  <tamar.christina@arm.com>

> 

> 	PR target/94396

> 	* common/config/aarch64/aarch64-common.c

> 	(aarch64_get_extension_string_for_isa_flags): Handle default flags.

> 

> gcc/testsuite/ChangeLog:

> 

> 2020-04-02  Tamar Christina  <tamar.christina@arm.com>

> 

> 	PR target/94396

> 	* gcc.target/aarch64/options_set_11.c: New test.

> 	* gcc.target/aarch64/options_set_12.c: New test.

> 	* gcc.target/aarch64/options_set_13.c: New test.

> 	* gcc.target/aarch64/options_set_14.c: New test.

> 	* gcc.target/aarch64/options_set_15.c: New test.

> 	* gcc.target/aarch64/options_set_16.c: New test.

> 	* gcc.target/aarch64/options_set_17.c: New test.

> 	* gcc.target/aarch64/options_set_18.c: New test.

> 	* gcc.target/aarch64/options_set_19.c: New test.

> 	* gcc.target/aarch64/options_set_20.c: New test.

> 	* gcc.target/aarch64/options_set_21.c: New test.

> 	* gcc.target/aarch64/options_set_22.c: New test.

> 	* gcc.target/aarch64/options_set_23.c: New test.

> 	* gcc.target/aarch64/options_set_24.c: New test.

> 	* gcc.target/aarch64/options_set_25.c: New test.

> 	* gcc.target/aarch64/options_set_26.c: New test.

> 


OK.

R.

PS.  It's canonicalize, not canonize (this is not saintly code ;-).

R.

Patch

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 8d24c140ee45c5e1b1313e6a85a8b8e44bb05405..0bddcc8c3e9282a957c5479b4df7f68058093bab 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -391,7 +391,22 @@  aarch64_get_extension_string_for_isa_flags (uint64_t isa_flags,
 	/* We remove all the dependent bits, to prevent them from being turned
 	   on twice.  This only works because we assume that all there are
 	   individual options to set all bits standalone.  */
-	isa_flag_bits &= ~opt->flags_on;
+
+	/* PR target/94396.
+
+	   For flags which would already imply a bit that's on by default (e.g
+	   fp16fml which implies +fp,+fp16) we must emit the flags that are not
+	   on by default.  i.e. in Armv8.4-a +fp16fml is default if +fp16.  So
+	   if a user passes armv8.4-a+fp16 (or +fp16fml) then we need to emit
+	   +fp16.  But if +fp16fml is used in an architecture where it is
+	   completely optional we only have to emit the canonical flag.  */
+	uint64_t toggle_bits = opt->flags_on & default_arch_flags;
+	/* Now check to see if the canonical flag is on by default.  If it
+	   is not then enabling it will enable all bits in flags_on.  */
+	if ((opt->flag_canonical & default_arch_flags) == 0)
+	  toggle_bits = opt->flags_on;
+
+	isa_flag_bits &= ~toggle_bits;
 	isa_flag_bits |= opt->flag_canonical;
       }
     }
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_11.c b/gcc/testsuite/gcc.target/aarch64/options_set_11.c
new file mode 100644
index 0000000000000000000000000000000000000000..d083bfdbd5c4ee0067607d506306a4271542c4d5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_11.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.2-a\+crc} } } */
+
+ /* FP is default on, no need to pass on to assembler.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_12.c b/gcc/testsuite/gcc.target/aarch64/options_set_12.c
new file mode 100644
index 0000000000000000000000000000000000000000..58a09fda2c1140bd63559f81280f41be5e1a2b17
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_12.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+fp16" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.2-a\+crc\+fp16} } } */
+
+ /* fp16 not default, should be emitted.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_13.c b/gcc/testsuite/gcc.target/aarch64/options_set_13.c
new file mode 100644
index 0000000000000000000000000000000000000000..2a517ecb58f87ca5653bb6aac7e2db12a1de0926
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_13.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+fp16+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.2-a\+crc\+fp16} } } */
+
+ /* FP is part of FP16, don't emit it.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_14.c b/gcc/testsuite/gcc.target/aarch64/options_set_14.c
new file mode 100644
index 0000000000000000000000000000000000000000..c192bf6cb63661f3d743b8c988ba2162e64c0959
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_14.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+fp16fml" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.2-a\+crc\+fp16fml} } } */
+
+ /* fmp16fml is smallest option to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_15.c b/gcc/testsuite/gcc.target/aarch64/options_set_15.c
new file mode 100644
index 0000000000000000000000000000000000000000..32ec3ea4643197e1bef052777a2717c73bef7d05
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_15.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+fp16fml+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.2-a\+crc\+fp16fml*} } } */
+
+ /* fp included in fp16fml, only emit latter.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_16.c b/gcc/testsuite/gcc.target/aarch64/options_set_16.c
new file mode 100644
index 0000000000000000000000000000000000000000..b45c01a915b99f9be77f206f862a69b4c81d0ff8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_16.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+fp16fml+fp16+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.2-a\+crc\+fp16fml} } } */
+
+ /* fp16fml is smallest options to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_17.c b/gcc/testsuite/gcc.target/aarch64/options_set_17.c
new file mode 100644
index 0000000000000000000000000000000000000000..c490e1f47a0a7a3adcbb7e96a3974d5651a023e8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_17.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.2-a+dotprod" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.2-a\+crc\+dotprod} } } */
+
+ /* dotprod needs to be emitted pre armv8.4.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_18.c b/gcc/testsuite/gcc.target/aarch64/options_set_18.c
new file mode 100644
index 0000000000000000000000000000000000000000..61587dbbd63a9803067553f6f5a4bf6ce86c090f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_18.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+dotprod" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc} } } */
+
+ /* dotprod is default in armv8.4-a, don't emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_19.c b/gcc/testsuite/gcc.target/aarch64/options_set_19.c
new file mode 100644
index 0000000000000000000000000000000000000000..72b58126182fa300bf3d065e8a9f18ea9a090438
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_19.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc} } } */
+
+ /* fp default, don't emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_20.c b/gcc/testsuite/gcc.target/aarch64/options_set_20.c
new file mode 100644
index 0000000000000000000000000000000000000000..b383e0aced2d2a12d8ec4ff021d27361b81356e0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_20.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp16" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc\+fp16} } } */
+
+ /* fp16 smallest set to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_21.c b/gcc/testsuite/gcc.target/aarch64/options_set_21.c
new file mode 100644
index 0000000000000000000000000000000000000000..19fcd6fda6e0687ae03eb84ba677b1fc3f438300
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_21.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp16+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc\+fp16} } } */
+
+ /* fp16 smallest set to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_22.c b/gcc/testsuite/gcc.target/aarch64/options_set_22.c
new file mode 100644
index 0000000000000000000000000000000000000000..77ae4089f3985917b314d5d548a9b9999ade8a15
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_22.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp16fml" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc\+fp16} } } */
+
+ /* fp16 smallest set to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_23.c b/gcc/testsuite/gcc.target/aarch64/options_set_23.c
new file mode 100644
index 0000000000000000000000000000000000000000..dee637c5d2cba1549861add1d81e697036aed047
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_23.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp16fml+fp" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc\+fp16} } } */
+
+ /* fp16 smallest set to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_24.c b/gcc/testsuite/gcc.target/aarch64/options_set_24.c
new file mode 100644
index 0000000000000000000000000000000000000000..54b0e3d4a8319144c1dd2fc8d736ca359788e738
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_24.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp16fml+fp16" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc\+fp16} } } */
+
+ /* fp16 smallest set to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_25.c b/gcc/testsuite/gcc.target/aarch64/options_set_25.c
new file mode 100644
index 0000000000000000000000000000000000000000..a3b2d63c06eb0bb3f1d59a4cdfb08d5918238c0c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_25.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp16fml+fp+fp16" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc\+fp16} } } */
+
+ /* fp16 smallest set to emit.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_26.c b/gcc/testsuite/gcc.target/aarch64/options_set_26.c
new file mode 100644
index 0000000000000000000000000000000000000000..b383e0aced2d2a12d8ec4ff021d27361b81356e0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/options_set_26.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-march=armv8.4-a+fp16" } */
+
+int main ()
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.arch armv8\.4-a\+crc\+fp16} } } */
+
+ /* fp16 smallest set to emit.  */