Patchwork [SH] PR 54685 - unsigned int comparison with 0x7FFFFFFF

login
register
mail settings
Submitter Oleg Endo
Date Oct. 6, 2012, 11:13 a.m.
Message ID <1349522019.9306.302.camel@yam-132-YW-E178-FTW>
Download mbox | patch
Permalink /patch/189679/
State New
Headers show

Comments

Oleg Endo - Oct. 6, 2012, 11:13 a.m.
Hello,

The attached patch improves comparisons such as
'unsigned int <= 0x7FFFFFFF' on SH.
As mentioned in the PR, for some reason, those comparisons do not go
through the cstore expander.  As a consequence the comparison doesn't
get the chance to be canonicalized by the target code and ends up as
'(~x) >> 31'.
I've not investigated this further and just fixed the symptoms on SH.  I
don't know whether it's also an issue on other targets.

Tested on rev 192142 with
make -k check RUNTESTFLAGS="--target_board=sh-sim
\{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"

and no new failures.
OK?

Cheers,
Oleg

gcc/ChangeLog:

	PR target/54685
	* config/sh/sh.md (one_cmplsi2): Make insn_and_split.  Add 
	manual combine matching for an insn sequence where a ge:SI
	pattern can be used.

testsuite/ChangeLog:

	PR target/54685
	* gcc.target/sh/pr54685.c: New.
Kaz Kojima - Oct. 8, 2012, 12:45 a.m.
Oleg Endo <oleg.endo@t-online.de> wrote:
> The attached patch improves comparisons such as
> 'unsigned int <= 0x7FFFFFFF' on SH.
> As mentioned in the PR, for some reason, those comparisons do not go
> through the cstore expander.  As a consequence the comparison doesn't
> get the chance to be canonicalized by the target code and ends up as
> '(~x) >> 31'.
> I've not investigated this further and just fixed the symptoms on SH.  I
> don't know whether it's also an issue on other targets.
> 
> Tested on rev 192142 with
> make -k check RUNTESTFLAGS="--target_board=sh-sim
> \{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"
> 
> and no new failures.
> OK?

I've run CSiBE with and without the patch for sh4-unknown-linux-gnu
at -O2.  Only one difference in the resulted sizes: jpeg-6b/jcphuff
increases 5336 bytes to 5340 bytes with the patch.  Could you look
into it?

Regards,
	kaz

Patch

Index: gcc/testsuite/gcc.target/sh/pr54685.c
===================================================================
--- gcc/testsuite/gcc.target/sh/pr54685.c	(revision 0)
+++ gcc/testsuite/gcc.target/sh/pr54685.c	(revision 0)
@@ -0,0 +1,58 @@ 
+/* Check that a comparison 'unsigned int <= 0x7FFFFFFF' results in code
+   utilizing the cmp/pz instruction.  */
+/* { dg-do compile { target "sh*-*-*" } } */
+/* { dg-options "-O1" } */
+/* { dg-skip-if "" { "sh*-*-*" } { "-m5*"} { "" } }  */
+/* { dg-final { scan-assembler-not "not" } } */
+/* { dg-final { scan-assembler-times "cmp/pz" 7 } } */
+/* { dg-final { scan-assembler-times "shll" 1 } } */
+/* { dg-final { scan-assembler-times "movt" 4 } } */
+
+int
+test_00 (unsigned int a)
+{
+  return !(a > 0x7FFFFFFF);
+}
+
+int
+test_01 (unsigned int a)
+{
+  return !(a > 0x7FFFFFFF) ? -5 : 10;
+}
+
+int
+test_02 (unsigned int a)
+{
+  /* 1x shll, 1x movt  */
+  return a >= 0x80000000;
+}
+
+int
+test_03 (unsigned int a)
+{
+  return a >= 0x80000000 ? -5 : 10;
+}
+
+int
+test_04 (unsigned int a)
+{
+  return a <= 0x7FFFFFFF;
+}
+
+int
+test_05 (unsigned int a)
+{
+  return a <= 0x7FFFFFFF ? -5 : 10;
+}
+
+int
+test_06 (unsigned int a)
+{
+  return a < 0x80000000;
+}
+
+int
+test_07 (unsigned int a)
+{
+  return a < 0x80000000 ? -5 : 10;
+}
Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md	(revision 192142)
+++ gcc/config/sh/sh.md	(working copy)
@@ -5188,11 +5188,61 @@ 
   "neg	%1,%0"
   [(set_attr "type" "arith")])
 
-(define_insn "one_cmplsi2"
+(define_insn_and_split "one_cmplsi2"
   [(set (match_operand:SI 0 "arith_reg_dest" "=r")
 	(not:SI (match_operand:SI 1 "arith_reg_operand" "r")))]
   "TARGET_SH1"
   "not	%1,%0"
+  "&& can_create_pseudo_p ()"
+  [(set (reg:SI T_REG) (ge:SI (match_dup 1) (const_int 0)))
+   (set (match_dup 0) (reg:SI T_REG))]
+{
+/* PR 54685
+   If the result of 'unsigned int <= 0x7FFFFFFF' ends up as the following
+   sequence:
+
+     (set (reg0) (not:SI (reg0) (reg1)))
+     (parallel [(set (reg2) (lshiftrt:SI (reg0) (const_int 31)))
+		(clobber (reg:SI T_REG))])
+
+   ... match and combine the sequence manually in the split pass after the
+   combine pass.  Notice that combine does try the target pattern of this
+   split, but if the pattern is added it interferes with other patterns, in
+   particular with the div0s comparisons.
+   This could also be done with a peephole but doing it here before register
+   allocation can save one temporary.
+   When we're here, the not:SI pattern obviously has been matched already
+   and we only have to see whether the following insn is the left shift.  */
+
+  rtx i = next_nonnote_insn_bb (curr_insn);
+  if (i == NULL_RTX || !NONJUMP_INSN_P (i))
+    FAIL;
+
+  rtx p = PATTERN (i);
+  if (GET_CODE (p) != PARALLEL || XVECLEN (p, 0) != 2)
+    FAIL;
+
+  rtx p0 = XVECEXP (p, 0, 0);
+  rtx p1 = XVECEXP (p, 0, 1);
+
+  if (/* (set (reg2) (lshiftrt:SI (reg0) (const_int 31)))  */
+      GET_CODE (p0) == SET
+      && GET_CODE (XEXP (p0, 1)) == LSHIFTRT
+      && REG_P (XEXP (XEXP (p0, 1), 0))
+      && REGNO (XEXP (XEXP (p0, 1), 0)) == REGNO (operands[0])
+      && CONST_INT_P (XEXP (XEXP (p0, 1), 1))
+      && INTVAL (XEXP (XEXP (p0, 1), 1)) == 31
+
+      /* (clobber (reg:SI T_REG))  */
+      && GET_CODE (p1) == CLOBBER && REG_P (XEXP (p1, 0))
+      && REGNO (XEXP (p1, 0)) == T_REG)
+    {
+      operands[0] = XEXP (p0, 0);
+      set_insn_deleted (i);
+    }
+  else
+    FAIL;
+}
   [(set_attr "type" "arith")])
 
 (define_expand "one_cmpldi2"