Re: [PATCH], PR target/78900, Fix PowerPC __float128 signbit

2017-01-03 Thread David Edelsohn
On Fri, Dec 30, 2016 at 3:54 PM, Michael Meissner
 wrote:
> The signbit-3.c test explicitly tests for the value coming from memory, a
> vector register, or a GPR.  Unfortunately, the code did not handle splitting 
> up
> the registers when the value was in a GPR.
>
> These patches add teh GPR support.  While I was editing the code, I also did
> some cleanup.
>
> I removed the Fsignbit mode attribute, since the only two modes used both use
> the same attribute.  This is a relic of the original code generation that also
> provided optimized signbit support for DFmode/SFmode.  Since the DFmode/SFmode
> got dropped (GCC 6 was in stage 3, and we needed to get signbit working for
> __float128 -- it already worked for DFmode/SFmode, but the code generation
> could be improved).
>
> I also noticed that use of signbit tended to generate sign or zero extension.
> Since the function only returns 0/1, I added combiner insns to eliminate the
> extra zero/sign extend.
>
> I have tested this on both big endian and little endian power8 systems.  The
> bootstrap and make check had no regressions.  Is this ok to put into the 
> trunk?
>
> The same error appears on GCC 6 as well.  Assuming the patch applys cleanly 
> and
> fixes the problem, can I install it on the GCC 6 branch as well after a burn 
> in
> period?
>
> 2016-12-30  Michael Meissner  
>
> PR target/78900
> * config/rs6000/rs6000.c (rs6000_split_signbit): Change some
> assertions.  Add support for doing the signbit if the IEEE 128-bit
> floating point value is in a GPR.
> * config/rs6000/rs6000.md (Fsignbit): Delete.
> (signbit2_dm): Delete using  and just use "wa".
> Update the length attribute if the value is in a GPR.
> (signbit2_dm_ext): Add combiner pattern to eliminate
> the sign or zero extension instruction, since the value is always
> 0/1.
> (signbit2_dm2): Delete using .

This patch is okay for trunk and okay for GCC 6 branch after a week or
two of no problems.

Thanks, David


[PATCH], PR target/78900, Fix PowerPC __float128 signbit

2016-12-30 Thread Michael Meissner
The signbit-3.c test explicitly tests for the value coming from memory, a
vector register, or a GPR.  Unfortunately, the code did not handle splitting up
the registers when the value was in a GPR.

These patches add teh GPR support.  While I was editing the code, I also did
some cleanup.

I removed the Fsignbit mode attribute, since the only two modes used both use
the same attribute.  This is a relic of the original code generation that also
provided optimized signbit support for DFmode/SFmode.  Since the DFmode/SFmode
got dropped (GCC 6 was in stage 3, and we needed to get signbit working for
__float128 -- it already worked for DFmode/SFmode, but the code generation
could be improved).

I also noticed that use of signbit tended to generate sign or zero extension.
Since the function only returns 0/1, I added combiner insns to eliminate the
extra zero/sign extend.

I have tested this on both big endian and little endian power8 systems.  The
bootstrap and make check had no regressions.  Is this ok to put into the trunk?

The same error appears on GCC 6 as well.  Assuming the patch applys cleanly and
fixes the problem, can I install it on the GCC 6 branch as well after a burn in
period?

2016-12-30  Michael Meissner  

PR target/78900
* config/rs6000/rs6000.c (rs6000_split_signbit): Change some
assertions.  Add support for doing the signbit if the IEEE 128-bit
floating point value is in a GPR.
* config/rs6000/rs6000.md (Fsignbit): Delete.
(signbit2_dm): Delete using  and just use "wa".
Update the length attribute if the value is in a GPR.
(signbit2_dm_ext): Add combiner pattern to eliminate
the sign or zero extension instruction, since the value is always
0/1.
(signbit2_dm2): Delete using .

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c  (revision 243966)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -25170,9 +25170,7 @@ rs6000_split_signbit (rtx dest, rtx src)
   rtx dest_di = (d_mode == DImode) ? dest : gen_lowpart (DImode, dest);
   rtx shift_reg = dest_di;
 
-  gcc_assert (REG_P (dest));
-  gcc_assert (REG_P (src) || MEM_P (src));
-  gcc_assert (s_mode == KFmode || s_mode == TFmode);
+  gcc_assert (FLOAT128_IEEE_P (s_mode) && TARGET_POWERPC64);
 
   if (MEM_P (src))
 {
@@ -25184,17 +25182,20 @@ rs6000_split_signbit (rtx dest, rtx src)
 
   else
 {
-  unsigned int r = REGNO (src);
+  unsigned int r = reg_or_subregno (src);
 
-  /* If this is a VSX register, generate the special mfvsrd instruction
-to get it in a GPR.  Until we support SF and DF modes, that will
-always be true.  */
-  gcc_assert (VSX_REGNO_P (r));
+  if (INT_REGNO_P (r))
+   shift_reg = gen_rtx_REG (DImode, r + (BYTES_BIG_ENDIAN == 0));
 
-  if (s_mode == KFmode)
-   emit_insn (gen_signbitkf2_dm2 (dest_di, src));
   else
-   emit_insn (gen_signbittf2_dm2 (dest_di, src));
+   {
+ /* Generate the special mfvsrd instruction to get it in a GPR.  */
+ gcc_assert (VSX_REGNO_P (r));
+ if (s_mode == KFmode)
+   emit_insn (gen_signbitkf2_dm2 (dest_di, src));
+ else
+   emit_insn (gen_signbittf2_dm2 (dest_di, src));
+   }
 }
 
   emit_insn (gen_lshrdi3 (dest_di, shift_reg, GEN_INT (63)));
Index: gcc/config/rs6000/rs6000.md
===
--- gcc/config/rs6000/rs6000.md (revision 243966)
+++ gcc/config/rs6000/rs6000.md (working copy)
@@ -518,9 +518,6 @@ (define_mode_iterator FLOAT128 [(KF "TAR
 (define_mode_iterator SIGNBIT [(KF "FLOAT128_VECTOR_P (KFmode)")
   (TF "FLOAT128_VECTOR_P (TFmode)")])
 
-(define_mode_attr Fsignbit [(KF "wa")
-(TF "wa")])
-
 ; Iterator for ISA 3.0 supported floating point types
 (define_mode_iterator FP_ISA3 [SF
   DF
@@ -4744,7 +4741,7 @@ (define_expand "copysign3"
 (define_insn_and_split "signbit2_dm"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r,r")
(unspec:SI
-[(match_operand:SIGNBIT 1 "input_operand" ",m,r")]
+[(match_operand:SIGNBIT 1 "input_operand" "wa,m,r")]
 UNSPEC_SIGNBIT))]
   "TARGET_POWERPC64 && TARGET_DIRECT_MOVE"
   "#"
@@ -4754,7 +4751,24 @@ (define_insn_and_split "signbit2_d
   rs6000_split_signbit (operands[0], operands[1]);
   DONE;
 }
- [(set_attr "length" "8,8,12")
+ [(set_attr "length" "8,8,4")
+  (set_attr "type" "mftgpr,load,integer")])
+
+(define_insn_and_split "*signbit2_dm_ext"
+  [(set (match_operand:DI 0 "gpc_reg_operand" "=r,r,r")
+   (any_extend:DI
+(unspec:SI
+ [(match_operand:SIGNBIT 1 "input_operand" "wa,m,r")]
+ UNSPEC_SIGNBIT)))]
+