Fix 70278 (LRA split_regs followup patch)

Fix 70278 (LRA split_regs followup patch)

[Bernd Schmidt]

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

This fixes an oversight in my previous patch here. I used biggest_mode 
in the assumption that if the reg was used in the function, it would be 
set to something other than VOIDmode, but that fails if we have a 
multiword access - only the first hard reg gets its biggest_mode 
assigned in that case.

Bootstrapped and tested on x86_64-linux, ran (just) the new arm testcase 
manually with arm-eabi. Ok?

(The testcase seems to be from glibc. Do we keep the copyright notices 
on the reduced form)?


Bernd

[-- Attachment #2: 70278.diff --]
[-- Type: text/x-patch, Size: 3936 bytes --]

	PR rtl-optimization/70278
	* lra-constraints.c (split_reg): Handle the case where biggest_mode is
	VOIDmode.

testsuite/
	* gcc.dg/torture/pr70278.c: New test.
	* gcc.target/arm/pr70278.c: New test.

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	(revision 234184)
+++ gcc/lra-constraints.c	(working copy)
@@ -4982,7 +4982,12 @@ split_reg (bool before_p, int original_r
       nregs = 1;
       mode = lra_reg_info[hard_regno].biggest_mode;
       machine_mode reg_rtx_mode = GET_MODE (regno_reg_rtx[hard_regno]);
-      if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (reg_rtx_mode))
+      /* A reg can have a biggest_mode of VOIDmode if it was only ever seen
+	 as part of a multi-word register.  In that case, or if the biggest
+	 mode was larger than a register, just use the reg_rtx.  Otherwise,
+	 limit the size to that of the biggest access in the function.  */
+      if (mode == VOIDmode
+	  || GET_MODE_SIZE (mode) > GET_MODE_SIZE (reg_rtx_mode))
 	{
 	  original_reg = regno_reg_rtx[hard_regno];
 	  mode = reg_rtx_mode;
Index: gcc/testsuite/gcc.dg/torture/pr70278.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr70278.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr70278.c	(working copy)
@@ -0,0 +1,37 @@
+/* { dg-do compile } */
+/*
+ * ====================================================
+ * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
+ *
+ * Developed at SunPro, a Sun Microsystems, Inc. business.
+ * Permission to use, copy, modify, and distribute this
+ * software is freely granted, provided that this notice 
+ * is preserved.
+ * ====================================================
+ */
+typedef union
+{
+  double value;
+  struct
+  {
+    unsigned int msw;
+  } parts;
+} ieee_double_shape_type;
+double __ieee754_hypot(double x, double y)
+{
+ double a=x,b=y,t1,t2,y1,y2,w;
+ int j,k,ha,hb;
+ do { ieee_double_shape_type gh_u; gh_u.value = (x); (ha) = gh_u.parts.msw; } while (0);;
+ if(hb > ha) {a=y;b=x;j=ha; ha=hb;hb=j;} else {a=x;b=y;}
+ if(ha > 0x5f300000) {
+    do { ieee_double_shape_type sh_u; sh_u.value = (a); sh_u.parts.msw = (ha); (a) = sh_u.value; } while (0);;
+ }
+ w = a-b;
+ if (w <= b)
+ {
+     t2 = a - t1;
+     w = t1*y1-(w*(-w)-(t1*y2+t2*b));
+ }
+ if(k!=0) {
+ } else return w;
+}
Index: gcc/testsuite/gcc.target/arm/pr70278.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr70278.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr70278.c	(working copy)
@@ -0,0 +1,41 @@
+/* { dg-do compile } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv4t" } } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" } { "" } } */
+/* { dg-options "-mthumb" } */
+/* { dg-add-options arm_arch_v4t } */
+/*
+ * ====================================================
+ * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
+ *
+ * Developed at SunPro, a Sun Microsystems, Inc. business.
+ * Permission to use, copy, modify, and distribute this
+ * software is freely granted, provided that this notice 
+ * is preserved.
+ * ====================================================
+ */
+typedef union
+{
+  double value;
+  struct
+  {
+    unsigned int msw;
+  } parts;
+} ieee_double_shape_type;
+double __ieee754_hypot(double x, double y)
+{
+ double a=x,b=y,t1,t2,y1,y2,w;
+ int j,k,ha,hb;
+ do { ieee_double_shape_type gh_u; gh_u.value = (x); (ha) = gh_u.parts.msw; } while (0);;
+ if(hb > ha) {a=y;b=x;j=ha; ha=hb;hb=j;} else {a=x;b=y;}
+ if(ha > 0x5f300000) {
+    do { ieee_double_shape_type sh_u; sh_u.value = (a); sh_u.parts.msw = (ha); (a) = sh_u.value; } while (0);;
+ }
+ w = a-b;
+ if (w <= b)
+ {
+     t2 = a - t1;
+     w = t1*y1-(w*(-w)-(t1*y2+t2*b));
+ }
+ if(k!=0) {
+ } else return w;
+}

Re: Fix 70278 (LRA split_regs followup patch)

[Jeff Law]

On 03/18/2016 06:25 AM, Bernd Schmidt wrote:
> This fixes an oversight in my previous patch here. I used biggest_mode
> in the assumption that if the reg was used in the function, it would be
> set to something other than VOIDmode, but that fails if we have a
> multiword access - only the first hard reg gets its biggest_mode
> assigned in that case.
>
> Bootstrapped and tested on x86_64-linux, ran (just) the new arm testcase
> manually with arm-eabi. Ok?
>
> (The testcase seems to be from glibc. Do we keep the copyright notices
> on the reduced form)?
I don't recall specific guidance on including the copyright notice on a 
reduced/derived test.

Given the actual copyright on the original code, ISTM the safest thing 
to do is keep the notice intact.

A long long time ago I receive guidance from the FSF WRT what could be 
included in the testsuite -- unfortunately I didn't keep that message. 
I probably should have.

>
> Bernd
>
> 70278.diff
>
>
> 	PR rtl-optimization/70278
> 	* lra-constraints.c (split_reg): Handle the case where biggest_mode is
> 	VOIDmode.
>
> testsuite/
> 	* gcc.dg/torture/pr70278.c: New test.
> 	* gcc.target/arm/pr70278.c: New test.
OK.
jeff

Re: Fix 70278 (LRA split_regs followup patch)

[Christophe Lyon]

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

On 18 March 2016 at 17:51, Jeff Law <law@redhat.com> wrote:
> On 03/18/2016 06:25 AM, Bernd Schmidt wrote:
>>
>> This fixes an oversight in my previous patch here. I used biggest_mode
>> in the assumption that if the reg was used in the function, it would be
>> set to something other than VOIDmode, but that fails if we have a
>> multiword access - only the first hard reg gets its biggest_mode
>> assigned in that case.
>>
>> Bootstrapped and tested on x86_64-linux, ran (just) the new arm testcase
>> manually with arm-eabi. Ok?
>>
>> (The testcase seems to be from glibc. Do we keep the copyright notices
>> on the reduced form)?
>
> I don't recall specific guidance on including the copyright notice on a
> reduced/derived test.
>
> Given the actual copyright on the original code, ISTM the safest thing to do
> is keep the notice intact.
>
> A long long time ago I receive guidance from the FSF WRT what could be
> included in the testsuite -- unfortunately I didn't keep that message. I
> probably should have.
>
>>
>> Bernd
>>
>> 70278.diff
>>
>>
>>         PR rtl-optimization/70278
>>         * lra-constraints.c (split_reg): Handle the case where
>> biggest_mode is
>>         VOIDmode.
>>
>> testsuite/
>>         * gcc.dg/torture/pr70278.c: New test.
>>         * gcc.target/arm/pr70278.c: New test.
>

The ARM test isn't sufficiently protected against non-compliant configurations,
and fails if GCC is configured for arm*linux-gnueabihf for instance
(see http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/234342/report-build-info.html)

The attached small patch fixes that by requiring arm_arch_v4t_multilib
effective target.

I used arm_arch_v4t_multilib instead of arm_arch_v4t because, as I
reported a long time ago
the later does not complain in some unsupported configuration because
the sample effective
target test does not contain actual code. In particular it's not
sufficient to reject thumb-1 with
hard-float.

OK?

Thanks,

Christophe.

> OK.
> jeff
>

[-- Attachment #2: pr70278.log.txt --]
[-- Type: text/plain, Size: 137 bytes --]

2016-03-22  Christophe Lyon  <christophe.lyon@linaro.org>

	* gcc.target/arm/pr70278.c: Require arm_arch_v4t_multilib
	effective target.

[-- Attachment #3: pr70278.patch.txt --]
[-- Type: text/plain, Size: 614 bytes --]

diff --git a/gcc/testsuite/gcc.target/arm/pr70278.c b/gcc/testsuite/gcc.target/arm/pr70278.c
index c44c07b..889f626 100644
--- a/gcc/testsuite/gcc.target/arm/pr70278.c
+++ b/gcc/testsuite/gcc.target/arm/pr70278.c
@@ -2,6 +2,7 @@
 /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv4t" } } */
 /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" } { "" } } */
 /* { dg-options "-mthumb" } */
+/* { dg-require-effective-target arm_arch_v4t_multilib } */
 /* { dg-add-options arm_arch_v4t } */
 /*
  * ====================================================

Re: Fix 70278 (LRA split_regs followup patch)

[Bernd Schmidt]

On 03/22/2016 10:24 AM, Christophe Lyon wrote:
>
> The ARM test isn't sufficiently protected against non-compliant configurations,
> and fails if GCC is configured for arm*linux-gnueabihf for instance
> (see http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/234342/report-build-info.html)
>
> The attached small patch fixes that by requiring arm_arch_v4t_multilib
> effective target.
>
> I used arm_arch_v4t_multilib instead of arm_arch_v4t because, as I
> reported a long time ago
> the later does not complain in some unsupported configuration because
> the sample effective
> target test does not contain actual code. In particular it's not
> sufficient to reject thumb-1 with
> hard-float.
>
> OK?

No objections from me, but I copied all this from the existing testcase 
ftest-armv4t-thumb.c, so I'm puzzled why that one doesn't fail.


Bernd

Re: Fix 70278 (LRA split_regs followup patch)

[Christophe Lyon]

On 22 March 2016 at 13:14, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 03/22/2016 10:24 AM, Christophe Lyon wrote:
>>
>>
>> The ARM test isn't sufficiently protected against non-compliant
>> configurations,
>> and fails if GCC is configured for arm*linux-gnueabihf for instance
>> (see
>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/234342/report-build-info.html)
>>
>> The attached small patch fixes that by requiring arm_arch_v4t_multilib
>> effective target.
>>
>> I used arm_arch_v4t_multilib instead of arm_arch_v4t because, as I
>> reported a long time ago
>> the later does not complain in some unsupported configuration because
>> the sample effective
>> target test does not contain actual code. In particular it's not
>> sufficient to reject thumb-1 with
>> hard-float.
>>
>> OK?
>
>
> No objections from me, but I copied all this from the existing testcase
> ftest-armv4t-thumb.c, so I'm puzzled why that one doesn't fail.
>

It's similar to why I tried to explain above: ftest-armv4t-thumb.c contains
only preprocessor tests, no actual code.

When the program contains code (or even a single global variable definition),
the compiler complains that" Thumb-1 hard-float VFP ABI" is not
implemented.

A long time ago, I submitted a patch to add some code to the
arm_arch_FUNC_ok effective target, but it was not accepted.

Christophe.

>
> Bernd