diff mbox

, PR target/78900, Fix PowerPC __float128 signbit

Message ID 20161230205454.GA25347@ibm-tiger.the-meissners.org
State New
Headers show

Commit Message

Michael Meissner Dec. 30, 2016, 8:54 p.m. UTC 
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  <meissner@linux.vnet.ibm.com>

	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.
	(signbit<mode>2_dm): Delete using <Fsignbit> and just use "wa".
	Update the length attribute if the value is in a GPR.
	(signbit<mode>2_dm_<su>ext): Add combiner pattern to eliminate
	the sign or zero extension instruction, since the value is always
	0/1.
	(signbit<mode>2_dm2): Delete using <Fsignbit>.

Comments

David Edelsohn Jan. 3, 2017, 7:06 p.m. UTC | #1 
On Fri, Dec 30, 2016 at 3:54 PM, Michael Meissner
<meissner@linux.vnet.ibm.com> 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  <meissner@linux.vnet.ibm.com>
>
>         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.
>         (signbit<mode>2_dm): Delete using <Fsignbit> and just use "wa".
>         Update the length attribute if the value is in a GPR.
>         (signbit<mode>2_dm_<su>ext): Add combiner pattern to eliminate
>         the sign or zero extension instruction, since the value is always
>         0/1.
>         (signbit<mode>2_dm2): Delete using <Fsignbit>.

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

Thanks, David
diff mbox

Patch

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 "copysign<mode>3"
 (define_insn_and_split "signbit<mode>2_dm"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r,r")
 	(unspec:SI
-	 [(match_operand:SIGNBIT 1 "input_operand" "<Fsignbit>,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 "signbit<mode>2_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 "*signbit<mode>2_dm_<su>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)))]
+  "TARGET_POWERPC64 && TARGET_DIRECT_MOVE"
+  "#"
+  "&& reload_completed"
+  [(const_int 0)]
+{
+  rs6000_split_signbit (operands[0], operands[1]);
+  DONE;
+}
+ [(set_attr "length" "8,8,4")
   (set_attr "type" "mftgpr,load,integer")])
 
 ;; MODES_TIEABLE_P doesn't allow DImode to be tied with the various floating
@@ -4762,7 +4776,7 @@  (define_insn_and_split "signbit<mode>2_d
 ;; special pattern to avoid using a normal movdi.
 (define_insn "signbit<mode>2_dm2"
   [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
-	(unspec:DI [(match_operand:SIGNBIT 1 "gpc_reg_operand" "<Fsignbit>")
+	(unspec:DI [(match_operand:SIGNBIT 1 "gpc_reg_operand" "wa")
 		    (const_int 0)]
 		   UNSPEC_SIGNBIT))]
   "TARGET_POWERPC64 && TARGET_DIRECT_MOVE"