unreliable/confusing dg-add-options arm_fp16_alternatives

Message ID orftdumdtb.fsf@livre.home
State New
Headers show
Series
  • unreliable/confusing dg-add-options arm_fp16_alternatives
Related show

Commit Message

Alexandre Oliva March 27, 2020, 7:56 a.m.
gcc.target/arm/fp16-aapcs-3.c is failing on one of our targets, and I'm
puzzled as to the reason for the failure and to the proper fix.

The test fails because, despite the dg-add-options in the subject,
-mfp16-format=alternative is not passed to the compiler, so type __fp16
is not recognized.

gcc.target/arm/fp16-aapcs-4.c used to have the same dg-add-options, but
that was changed to an explicit -mfp16-format=alternative.
commit e70bbc6f1f55a9532219812309ec22b04b539367  r240655
so I'm tempted to make a similar change to fp16-aapcs-3.c:


whereas add_options_for_arm_fp16_alternative requires
check_effective_target_arm_fp16_ok, which presumably fails, causing both
dg-add-options to return without adding any options, and also causes
arm_fp16_hw requires to fail.


So, here are some potential fixes:

- install the patchlet for fp16-aapcs-3.c above, and be done with it

- add an arm_fp16_hw requirement to this test

- add to check_effective_target_arm_fp16_alternative_ok_nocache above a
  check for arm_fp16, besides arm32.

Unrelated potential fix, assuming it's a cut&pasto rather than intended:

- drop the et_arm_neon_fp16_flags settings from the unrelated (?)
  check_effective_target_arm_fp16_alternative_ok_nocache

Thanks in advance for signaling the preferred way to fix this.

-- 
Alexandre Oliva, freedom fighter    he/him    https://FSFLA.org/blogs/lxo/
Free Software Evangelist              Stallman was right, but he's left :(
GNU Toolchain Engineer           Live long and free, and prosper ethically

Comments

Alexandre Oliva April 8, 2020, 3:27 p.m. | #1
On Mar 27, 2020, Alexandre Oliva <oliva@adacore.com> wrote:

> So, here are some potential fixes:


> - install the patchlet for fp16-aapcs-3.c above, and be done with it


> - add an arm_fp16_hw requirement to this test


> - add to check_effective_target_arm_fp16_alternative_ok_nocache above a

>   check for arm_fp16, besides arm32.


> Unrelated potential fix, assuming it's a cut&pasto rather than intended:


> - drop the et_arm_neon_fp16_flags settings from the unrelated (?)

>   check_effective_target_arm_fp16_alternative_ok_nocache



Here's the patchlet turned into a patch submission:


add missing fp16 options

dg-require-effective-target arm_fp16_alternative_ok may pass even when
arm_fp16_ok doesn't, and the latter's failure inhibits dg-add-options
arm_fp16_alternative.  Requiring arm_fp16_ok would disable the test,
but if we just pass it the -mfp16-format=alternative option, it passes
even without arm_fp16_ok.  Sibling test fp16-aapcs-4.c underwent a
similar change, so I'm proposing the explicit option to fp16-aapcs-3.c
as well.

Tested with an arm-eabi cross compiler configured for a machine with
-mfloat-abi=hard -mfpu=vfpv3-d16.  Ok to install?


for  gcc/testsuite/ChangeLog

	* gcc.target/arm/fp16-aapcs-3.c: Explicitly use the
	-mfp16-format=alternative option.
---
 gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c b/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c
index 56a3ae2..858181c 100644
--- a/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c
+++ b/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c
@@ -1,8 +1,7 @@
 /* { dg-do compile }  */
 /* { dg-require-effective-target arm_hard_vfp_ok }  */
 /* { dg-require-effective-target arm_fp16_alternative_ok } */
-/* { dg-options "-O2" }  */
-/* { dg-add-options arm_fp16_alternative } */
+/* { dg-options "-O2 -mfp16-format=alternative" }  */
 
 /* Test __fp16 arguments and return value in registers (hard-float).  */
 


-- 
Alexandre Oliva, freedom fighter    he/him    https://FSFLA.org/blogs/lxo/
Free Software Evangelist              Stallman was right, but he's left :(
GNU Toolchain Engineer           Live long and free, and prosper ethically
Feng Xue OS via Gcc-patches April 8, 2020, 4:04 p.m. | #2
On Wed, 2020-04-08 at 12:27 -0300, Alexandre Oliva wrote:
> On Mar 27, 2020, Alexandre Oliva <oliva@adacore.com> wrote:

> 

> > So, here are some potential fixes:

> > - install the patchlet for fp16-aapcs-3.c above, and be done with it

> > - add an arm_fp16_hw requirement to this test

> > - add to check_effective_target_arm_fp16_alternative_ok_nocache above a

> >   check for arm_fp16, besides arm32.

> > Unrelated potential fix, assuming it's a cut&pasto rather than intended:

> > - drop the et_arm_neon_fp16_flags settings from the unrelated (?)

> >   check_effective_target_arm_fp16_alternative_ok_nocache

> 

> Here's the patchlet turned into a patch submission:

> 

> 

> add missing fp16 options

> 

> dg-require-effective-target arm_fp16_alternative_ok may pass even when

> arm_fp16_ok doesn't, and the latter's failure inhibits dg-add-options

> arm_fp16_alternative.  Requiring arm_fp16_ok would disable the test,

> but if we just pass it the -mfp16-format=alternative option, it passes

> even without arm_fp16_ok.  Sibling test fp16-aapcs-4.c underwent a

> similar change, so I'm proposing the explicit option to fp16-aapcs-3.c

> as well.

> 

> Tested with an arm-eabi cross compiler configured for a machine with

> -mfloat-abi=hard -mfpu=vfpv3-d16.  Ok to install?

> 

> 

> for  gcc/testsuite/ChangeLog

> 

> 	* gcc.target/arm/fp16-aapcs-3.c: Explicitly use the

> 	-mfp16-format=alternative option.

OK
jeff
>

Patch

diff --git a/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c b/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c
index 56a3ae2..858181c 100644
--- a/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c
+++ b/gcc/testsuite/gcc.target/arm/fp16-aapcs-3.c
@@ -1,8 +1,7 @@ 
 /* { dg-do compile }  */
 /* { dg-require-effective-target arm_hard_vfp_ok }  */
 /* { dg-require-effective-target arm_fp16_alternative_ok } */
-/* { dg-options "-O2" }  */
-/* { dg-add-options arm_fp16_alternative } */
+/* { dg-options "-O2 -mfp16-format=alternative" }  */
 
 /* Test __fp16 arguments and return value in registers (hard-float).  */
 

With the change above, the test compiled and passed.  However, 
other tests with such dg-add-options requests (aapcs/vfp2[2-5].c) all have
/* { dg-require-effective-target arm_fp16_hw }  */
and, presumably for this unmet requirement, come out as UNSUPPORTED.
I imagine arm_fp16_alternative_ok should imply this.


I tried to figure out why the dg-add-options wasn't working, and I found
something very puzzling: there appears to be a disconnect between
/* { dg-require-effective-target arm_fp16_alternative_ok } */
and /* { dg-add-options arm_fp16_alternative } */

You'd think the latter would work if the former passed, but they seem to
be unrelated.  check_effective_target_arm_fp16_alternative_ok_nocache
seems to be setting a flags variable that pertains to something else
(cut&pasto?  intended?  the patchlet below is just for context, not
meant for inclusion as is), and that does not require an arm_fp16
effective target:

diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 3654e7b..9000721 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -4156,11 +4156,11 @@  proc add_options_for_aarch64_sve { flags } {
 
 # Return 1 if this is an ARM target supporting the FP16 alternative
 # format.  Some multilibs may be incompatible with the options needed.  Also
-# set et_arm_neon_fp16_flags to the best options to add.
+# set et_arm_fp16_flags to the best options to add.
 
 proc check_effective_target_arm_fp16_alternative_ok_nocache { } {
-    global et_arm_neon_fp16_flags
-    set et_arm_neon_fp16_flags ""
+    global et_arm_fp16_flags
+    set et_arm_fp16_flags ""
     if { [check_effective_target_arm32] } {
 	foreach flags {"" "-mfloat-abi=softfp" "-mfpu=neon-fp16"
 		       "-mfpu=neon-fp16 -mfloat-abi=softfp"} {
@@ -4170,7 +4170,7 @@  proc check_effective_target_arm_fp16_alternative_ok_nocache { } {
 		#error __ARM_FP16_FORMAT_ALTERNATIVE not defined
 		#endif
 	    } "$flags -mfp16-format=alternative"] } {
-		set et_arm_neon_fp16_flags "$flags -mfp16-format=alternative"
+		set et_arm_fp16_flags "$flags -mfp16-format=alternative"
 		return 1
 	    }
 	}