Re: [PATCH][ARM] PR target/66731 Fix vnmul insn with -frounding-math

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

Hi Szabolcs,

On 24/07/15 12:10, Szabolcs Nagy wrote:
> (-a)*b should not be compiled to vnmul a,b with -frounding-math.
> Added a new -(a*b) pattern for vnmul and the old one is only
> used if !flag_rounding_math.  Updated the costs too.
>
> This is the ARM version of
> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00300.html
>
> Tested with arm-none-linux-gnueabihf cross compiler.
> is this OK?
>
> gcc/Changelog:
>
> 2015-07-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	PR target/66731
> 	* config/arm/arm.md (muldf3negdf_vfp): Handle -frounding-math.
> 	(mulsf3negsf_vfp): Likewise.

This entry is misleading. You disable two existing patterns
for flag_rounding_math and you add two new patterns. The
entry should reflect that.
> 	* config/arm/arm.c (arm_new_rtx_costs): Fix NEG cost for VNMUL,
> 	fix MULT cost with -frounding-math.
>
> gcc/testsuite/Changelog:
>
> 2015-07-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	PR target/66731
> 	* gcc.target/arm/vnmul-1.c: New.
> 	* gcc.target/arm/vnmul-2.c: New.
> 	* gcc.target/arm/vnmul-3.c: New.
> 	* gcc.target/arm/vnmul-4.c: New.

diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
index f62ff79..214c48c 100644
--- a/gcc/config/arm/vfp.md
+++ b/gcc/config/arm/vfp.md
@@ -770,6 +770,17 @@
   [(set (match_operand:SF		   0 "s_register_operand" "=t")
 	(mult:SF (neg:SF (match_operand:SF 1 "s_register_operand" "t"))
 		 (match_operand:SF	   2 "s_register_operand" "t")))]
+  "TARGET_32BIT && TARGET_HARD_FLOAT && TARGET_VFP && !flag_rounding_math"
+  "vnmul%?.f32\\t%0, %1, %2"
+  [(set_attr "predicable" "yes")
+   (set_attr "predicable_short_it" "no")
+   (set_attr "type" "fmuls")]
+)
+
+(define_insn "*mulsf3negsf_vfp"
+  [(set (match_operand:SF		   0 "s_register_operand" "=t")
+	(neg:SF (mult:SF (match_operand:SF 1 "s_register_operand" "t")
+		 (match_operand:SF	   2 "s_register_operand" "t"))))]
   "TARGET_32BIT && TARGET_HARD_FLOAT && TARGET_VFP"
   "vnmul%?.f32\\t%0, %1, %2"
   [(set_attr "predicable" "yes")


Can you give the new pattern a different name to reflect that
the neg is on the outside? Something like *negmulsf3_vfp.

diff --git a/gcc/testsuite/gcc.target/arm/vnmul-1.c b/gcc/testsuite/gcc.target/arm/vnmul-1.c
new file mode 100644
index 0000000..0b4ca2c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/vnmul-1.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_vfp_ok } */
+/* { dg-skip-if "need fp instructions" { *-*-* } { "-mfloat-abi=soft" } { "" } } */
+/* { dg-options "-O2 -mfpu=vfp -mfloat-abi=hard" } */

Can you please add an explicit -fno-rounding-math here? That way we get a hint as to
why these tests exist. Alternatively, you can rename the tests to be pr66731_1.c,
pr66731_2.c etc. That way in the future we'll know what issue they're testing for.


diff --git a/gcc/testsuite/gcc.target/arm/vnmul-2.c b/gcc/testsuite/gcc.target/arm/vnmul-2.c
new file mode 100644
index 0000000..f9a8a5c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/vnmul-2.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_vfp_ok } */
+/* { dg-skip-if "need fp instructions" { *-*-* } { "-mfloat-abi=soft" } { "" } } */
+/* { dg-options "-O2 -frounding-math -mfpu=vfp -mfloat-abi=hard" } */
+
+double
+foo_d (double a, double b)
+{
+  /* { dg-final { scan-assembler "vneg\.f64" } } */
+  /* { dg-final { scan-assembler "vmul\.f64" } } */
+  return -a * b;
+}
+
+float
+foo_s (float a, float b)
+{
+  /* { dg-final { scan-assembler "vneg\.f32" } } */
+  /* { dg-final { scan-assembler "vmul\.f32" } } */
+  return -a * b;
+}

I'd prefer if you just do a scan-assembler not "vnmul", which is what this
patch really fixes. Whether the midend decides to use a pair of vneg+vmul
is tangential to this patch, it's the vnmul that we're trying to avoid.


Thanks,
Kyrill