[PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Chung-Lin Tang]

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

This patch is the main bulk of this submission. It modifies the compare
combining part of try_combine(), adding a call of
CANONICALIZE_COMPARISON into the entire logic.

Also, instead of testing for XEXP(SET_SRC(PATTERN(i3)),1) == const0_rtx
at the top, it now allows CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)),
tries to adjust it by simplify_compare_const() from the last patch, and
then tests if op1 == const0_rtx. This is a small improvement in some cases.

(if you remove the call to simplify_compare_const(), and if
CANONICALIZE_COMPARISON is not defined for the target, then this entire
patch should be an 'idempotent patch', nothing should be changed to the
effective combine results)

One issue that I would like to RFC, is the use of
CANONICALIZE_COMPARISON here; I'm afraid I might be abusing it. Since
added_sets_2 is set here, the value of i2src/op0 is needed afterwards.
This might not be conformant to the description of
CANONICALIZE_COMPARISON in the internals manual, which doesn't say it
can't trash op0 under arbitrary conditions while only preserving the
comparison result.

OTOH, I don't see another suitable macro/hook (with a close enough
meaning), and the current definitions of CANONICALIZE_COMPARISON across
the targets do not seem to clash with my use here. Does this macro use
look okay, or does this justify creating a new one?

Thanks,
Chung-Lin

[-- Attachment #2: 2-try_combine.diff --]
[-- Type: text/plain, Size: 4577 bytes --]

Index: combine.c
===================================================================
--- combine.c	(revision 172860)
+++ combine.c	(working copy)
@@ -3046,58 +3047,89 @@
 
   if (i1 == 0 && added_sets_2 && GET_CODE (PATTERN (i3)) == SET
       && GET_CODE (SET_SRC (PATTERN (i3))) == COMPARE
-      && XEXP (SET_SRC (PATTERN (i3)), 1) == const0_rtx
+      && CONST_INT_P (XEXP (SET_SRC (PATTERN (i3)), 1))
       && rtx_equal_p (XEXP (SET_SRC (PATTERN (i3)), 0), i2dest))
     {
-#ifdef SELECT_CC_MODE
-      rtx *cc_use;
-      enum machine_mode compare_mode;
-#endif
+      rtx newpat_dest;
+      rtx *cc_use_loc = NULL, cc_use_insn = NULL_RTX;
+      rtx op0 = i2src, op1 = XEXP (SET_SRC (PATTERN (i3)), 1);
+      enum machine_mode compare_mode, orig_compare_mode;
+      enum rtx_code compare_code = UNKNOWN, orig_compare_code = UNKNOWN;
 
       newpat = PATTERN (i3);
-      SUBST (XEXP (SET_SRC (newpat), 0), i2src);
+      newpat_dest = SET_DEST (newpat);
+      compare_mode = orig_compare_mode = GET_MODE (newpat_dest);
 
-      i2_is_used = 1;
-
-#ifdef SELECT_CC_MODE
-      /* See if a COMPARE with the operand we substituted in should be done
-	 with the mode that is currently being used.  If not, do the same
-	 processing we do in `subst' for a SET; namely, if the destination
-	 is used only once, try to replace it with a register of the proper
-	 mode and also replace the COMPARE.  */
       if (undobuf.other_insn == 0
-	  && (cc_use = find_single_use (SET_DEST (newpat), i3,
-					&undobuf.other_insn))
-	  && ((compare_mode = SELECT_CC_MODE (GET_CODE (*cc_use),
-					      i2src, const0_rtx))
-	      != GET_MODE (SET_DEST (newpat))))
+	  && (cc_use_loc = find_single_use (SET_DEST (newpat), i3,
+					    &cc_use_insn)))
 	{
-	  if (can_change_dest_mode (SET_DEST (newpat), added_sets_2,
-				    compare_mode))
+	  compare_code = orig_compare_code = GET_CODE (*cc_use_loc);
+	  compare_code = simplify_compare_const (compare_code,
+						 op0, &op1);
+#ifdef CANONICALIZE_COMPARISON
+	  CANONICALIZE_COMPARISON (compare_code, op0, op1);
+#endif
+	}
+
+      /* Do the rest only if op1 is const0_rtx, which may be the
+	 result of simplification.  */
+      if (op1 == const0_rtx)
+	{
+	  if (cc_use_loc)
 	    {
-	      unsigned int regno = REGNO (SET_DEST (newpat));
-	      rtx new_dest;
-
-	      if (regno < FIRST_PSEUDO_REGISTER)
-		new_dest = gen_rtx_REG (compare_mode, regno);
-	      else
+#ifdef SELECT_CC_MODE
+	      enum machine_mode new_mode
+		= SELECT_CC_MODE (compare_code, op0, op1);
+	      if (new_mode != orig_compare_mode
+		  && can_change_dest_mode (SET_DEST (newpat),
+					   added_sets_2, new_mode))
 		{
-		  SUBST_MODE (regno_reg_rtx[regno], compare_mode);
-		  new_dest = regno_reg_rtx[regno];
+		  unsigned int regno = REGNO (newpat_dest);
+		  compare_mode = new_mode;
+		  if (regno < FIRST_PSEUDO_REGISTER)
+		    newpat_dest = gen_rtx_REG (compare_mode, regno);
+		  else
+		    {
+		      SUBST_MODE (regno_reg_rtx[regno], compare_mode);
+		      newpat_dest = regno_reg_rtx[regno];
+		    }
 		}
+#endif
+	      /* Cases for modifying the CC-using comparison.  */
+	      if (compare_code != orig_compare_code
+		  /* ??? Do we need to verify the zero rtx?  */
+		  && XEXP (*cc_use_loc, 1) == const0_rtx)
+		{
+		  /* Replace cc_use_loc with entire new RTX.  */
+		  SUBST (*cc_use_loc,
+			 gen_rtx_fmt_ee (compare_code, compare_mode,
+					 newpat_dest, const0_rtx));
+		  undobuf.other_insn = cc_use_insn;
+		}
+	      else if (compare_mode != orig_compare_mode)
+		{
+		  /* Just replace the CC reg with a new mode.  */
+		  SUBST (XEXP (*cc_use_loc, 0), newpat_dest);
+		  undobuf.other_insn = cc_use_insn;
+		}	      
+	    }
 
-	      SUBST (SET_DEST (newpat), new_dest);
-	      SUBST (XEXP (*cc_use, 0), new_dest);
-	      SUBST (SET_SRC (newpat),
-		     gen_rtx_COMPARE (compare_mode, i2src, const0_rtx));
-	    }
-	  else
-	    undobuf.other_insn = 0;
+	  /* Create new reg:CC if the CC mode has been altered.  */
+	  if (compare_mode != orig_compare_mode)
+	    SUBST (SET_DEST (newpat), newpat_dest);
+	  /* This is always done to propagate i2src into newpat.  */
+	  SUBST (SET_SRC (newpat),
+		 gen_rtx_COMPARE (compare_mode, op0, op1));
+	  /* Create new version of i2pat if needed.  */
+	  if (! rtx_equal_p (i2src, op0))
+	    i2pat = gen_rtx_SET (VOIDmode, i2dest, op0);
+	  i2_is_used = 1;
 	}
-#endif
     }
-  else
 #endif
+
+  if (i2_is_used == 0)
     {
       /* It is possible that the source of I2 or I1 may be performing
 	 an unneeded operation, such as a ZERO_EXTEND of something

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/22/11 09:21, Chung-Lin Tang wrote:
> This patch is the main bulk of this submission. It modifies the compare
> combining part of try_combine(), adding a call of
> CANONICALIZE_COMPARISON into the entire logic.
> 
> Also, instead of testing for XEXP(SET_SRC(PATTERN(i3)),1) == const0_rtx
> at the top, it now allows CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)),
> tries to adjust it by simplify_compare_const() from the last patch, and
> then tests if op1 == const0_rtx. This is a small improvement in some cases.
> 
> (if you remove the call to simplify_compare_const(), and if
> CANONICALIZE_COMPARISON is not defined for the target, then this entire
> patch should be an 'idempotent patch', nothing should be changed to the
> effective combine results)
> 
> One issue that I would like to RFC, is the use of
> CANONICALIZE_COMPARISON here; I'm afraid I might be abusing it. Since
> added_sets_2 is set here, the value of i2src/op0 is needed afterwards.
> This might not be conformant to the description of
> CANONICALIZE_COMPARISON in the internals manual, which doesn't say it
> can't trash op0 under arbitrary conditions while only preserving the
> comparison result.
> 
> OTOH, I don't see another suitable macro/hook (with a close enough
> meaning), and the current definitions of CANONICALIZE_COMPARISON across
> the targets do not seem to clash with my use here. Does this macro use
> look okay, or does this justify creating a new one?
You seem to have removed the block comment (which was originally within
a SELECT_CC_MODE block).  I can see why, but ISTM you need some comments
to improve the readability of this code.  The code in the if
(cc_use_loc) block, it appears you're updating the cc user, a comment to
that effect and why is appropriate.

Following that block, you're actually changing the current insn, again a
comment indicating that would be helpful in making the code easier to read.

With regards to CANONICALIZE_COMPARISON; I think you're OK.  As far as I
can tell, you aren't changing the value of op0, you're just changing its
form.

I've generally found that avoiding const0_rtx is wise.  Instead I
suggest using CONST0_RTX (mode) to get the constant in the proper mode.

Given the tangled mess this code happens to be, could you please do a
bootstrap test on target with and without SELECT_CC_MODE.  x86 would be
a great example of the former, powerpc the latter (use the build farm if
you need to).   For ppc, just bootstrapping would be fine; I don't think
a full regression test is needed, just some verification that we don't
break ppc build should be sufficient.

jeff

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNtbiDAAoJEBRtltQi2kC7l0AH/3Vy479EufdUs2JAWJDnEdE1
TZ1Pa6zHhbb/hx6N8Jk1xgB6xM2QhzTCzjCFKVudQVNHxGx1hA0wkCN0jOsBmr91
pifBPmqr5a+w1bH4pfVMaG7XWqSMJmQP9Y02cAIbbB42TlX+47jAzfrEVaz2jRBX
9C3FetAZPSwhBlRiCzH3EC1Psoqs6NypuZgclUzdkOGoXTELVpzK7sxj7N0Vzf0O
Ubg3yStIpNFM+Kp+6g6yOjg+Q90gSCvR+EPM6jMMyFOxvykJ9RjTiIrvWklloqLO
Ez9hFd+i51h3px0xEdLfifRtIYHn47uj+4EBe4b5wXNJc+hQYvEa8gX7FVD8YXw=
=fQxG
-----END PGP SIGNATURE-----

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Chung-Lin Tang]

On 2011/4/26 02:08 AM, Jeff Law wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/22/11 09:21, Chung-Lin Tang wrote:
>> This patch is the main bulk of this submission. It modifies the compare
>> combining part of try_combine(), adding a call of
>> CANONICALIZE_COMPARISON into the entire logic.
>>
>> Also, instead of testing for XEXP(SET_SRC(PATTERN(i3)),1) == const0_rtx
>> at the top, it now allows CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)),
>> tries to adjust it by simplify_compare_const() from the last patch, and
>> then tests if op1 == const0_rtx. This is a small improvement in some cases.
>>
>> (if you remove the call to simplify_compare_const(), and if
>> CANONICALIZE_COMPARISON is not defined for the target, then this entire
>> patch should be an 'idempotent patch', nothing should be changed to the
>> effective combine results)
>>
>> One issue that I would like to RFC, is the use of
>> CANONICALIZE_COMPARISON here; I'm afraid I might be abusing it. Since
>> added_sets_2 is set here, the value of i2src/op0 is needed afterwards.
>> This might not be conformant to the description of
>> CANONICALIZE_COMPARISON in the internals manual, which doesn't say it
>> can't trash op0 under arbitrary conditions while only preserving the
>> comparison result.
>>
>> OTOH, I don't see another suitable macro/hook (with a close enough
>> meaning), and the current definitions of CANONICALIZE_COMPARISON across
>> the targets do not seem to clash with my use here. Does this macro use
>> look okay, or does this justify creating a new one?

Hi Jeff, thanks for reviewing this quite convoluted patch :)

> You seem to have removed the block comment (which was originally within
> a SELECT_CC_MODE block).  I can see why, but ISTM you need some comments
> to improve the readability of this code.  The code in the if
> (cc_use_loc) block, it appears you're updating the cc user, a comment to
> that effect and why is appropriate.
> 
> Following that block, you're actually changing the current insn, again a
> comment indicating that would be helpful in making the code easier to read.
> 

Sure, I'll try to add more clearer comments; and maybe someone else
could also see if my explanations are consistent with the transforms
intended.

> With regards to CANONICALIZE_COMPARISON; I think you're OK.  As far as I
> can tell, you aren't changing the value of op0, you're just changing its
> form.

Yes I'm not doing anything weird here, although the definition of the
CANONICALIZE_COMPARISON macro does not seem to forbid (other ports) from
doing so, as long as the comparison result is equivalent. It is
currently called only at the end of simplify_comparison(), which itself
possibly modifies the compare operands.

Considering not a lot of targets currently use CANONICALIZE_COMPARISON,
maybe it would be feasible to slightly change its interface? Maybe
adding a bool parameter to indicate whether the individual operands have
to be retained equivalent.

> 
> I've generally found that avoiding const0_rtx is wise.  Instead I
> suggest using CONST0_RTX (mode) to get the constant in the proper mode.
> 

Do you mean the (op1 == const0_rtx) test?

> Given the tangled mess this code happens to be, could you please do a
> bootstrap test on target with and without SELECT_CC_MODE.  x86 would be
> a great example of the former, powerpc the latter (use the build farm if
> you need to).   For ppc, just bootstrapping would be fine; I don't think
> a full regression test is needed, just some verification that we don't
> break ppc build should be sufficient.
> 

Bootstrap and test on x86 is completed already, both 32 and 64 bit.
Bootstrap on ARM itself is also completed, using a Pandaboard. I'll try
doing a ppc build too.

Thanks,
Chung-Lin

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/26/11 05:44, Chung-Lin Tang wrote:
> 
> Hi Jeff, thanks for reviewing this quite convoluted patch :)
FWIW, I don't think it's necessarily your patch that is convoluted, but
instead the original code.  It's also the case that I haven't spent
nearly as much time in the combiner as I have in other parts of GCC.


> Yes I'm not doing anything weird here, although the definition of the
> CANONICALIZE_COMPARISON macro does not seem to forbid (other ports) from
> doing so, as long as the comparison result is equivalent. It is
> currently called only at the end of simplify_comparison(), which itself
> possibly modifies the compare operands.
> 
> Considering not a lot of targets currently use CANONICALIZE_COMPARISON,
> maybe it would be feasible to slightly change its interface? Maybe
> adding a bool parameter to indicate whether the individual operands have
> to be retained equivalent.
That can certainly be done if necessary.  I'm just not sure it's needed,
at least not at this time.  If you want to make that change, feel free
to submit it.

> 
>>
>> I've generally found that avoiding const0_rtx is wise.  Instead I
>> suggest using CONST0_RTX (mode) to get the constant in the proper mode.
>>
> 
> Do you mean the (op1 == const0_rtx) test?
Yes.  It's a fairly minor issue.


> 
>> Given the tangled mess this code happens to be, could you please do a
>> bootstrap test on target with and without SELECT_CC_MODE.  x86 would be
>> a great example of the former, powerpc the latter (use the build farm if
>> you need to).   For ppc, just bootstrapping would be fine; I don't think
>> a full regression test is needed, just some verification that we don't
>> break ppc build should be sufficient.
>>
> 
> Bootstrap and test on x86 is completed already, both 32 and 64 bit.
> Bootstrap on ARM itself is also completed, using a Pandaboard. I'll try
> doing a ppc build too.
Thanks. Given that x86 & ARM are both SELECT_CC_MODE targets, I suspect
they're OK.  I mostly want a sniff test on a target that doesn't define
SELECT_CC_MODE (ppc for example).

Thanks,
jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNuuDyAAoJEBRtltQi2kC7gMQH/3hANrSFa3MSCTUvNxaiv9sy
HLoVYgcVXxNCNwYm/uSxZ3titiqyqB7ySrYPEezXCqdaNJeUSk+5v9w23wszyoel
bITxU6FC1P9wgHCRVNc9NsxpBJeCZleDSPHcrqAdbW8yO6J59qtaaRAlsBjgz11f
Isg1X+apG0Cy6DZ8knNRDVv9+HyJNLTYCg+90sSQv2Of1obp0b34sq/C2e03+oHz
gVBkgCvkLazcvYxLrdyCgXfyXvNMbMfSeVclJzpikJZAOAJBCwceYd16wg9ohBZR
y1g31oZ4teYcLz6c+yWEiWmZpVbeLnZSB9/bkvwP9lrQwkmKyxP33kdYHl/VP1E=
=ZZwe
-----END PGP SIGNATURE-----

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Chung-Lin Tang]

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

Hi Jeff,
I have verified the patch with a native bootstrap + testsuite run on
powerpc-linux (32-bit), results were clean.

Attached is a single patch with the 1+2 combine parts together, with
comments updated. Please check if they feel descriptive enough.

I haven't updated the CANONICALIZE_COMPARISON stuff, as we discussed it
doesn't look like absolutely needed right now. As for the const0_rtx
compare, because the entire case is guarded by a CONST_INT_P, I think it
should be safe.

Is this now okay for trunk?

Thanks,
Chung-Lin

On 2011/4/30 12:01 AM, Jeff Law wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/26/11 05:44, Chung-Lin Tang wrote:
>>
>> Hi Jeff, thanks for reviewing this quite convoluted patch :)
> FWIW, I don't think it's necessarily your patch that is convoluted, but
> instead the original code.  It's also the case that I haven't spent
> nearly as much time in the combiner as I have in other parts of GCC.
> 
> 
>> Yes I'm not doing anything weird here, although the definition of the
>> CANONICALIZE_COMPARISON macro does not seem to forbid (other ports) from
>> doing so, as long as the comparison result is equivalent. It is
>> currently called only at the end of simplify_comparison(), which itself
>> possibly modifies the compare operands.
>>
>> Considering not a lot of targets currently use CANONICALIZE_COMPARISON,
>> maybe it would be feasible to slightly change its interface? Maybe
>> adding a bool parameter to indicate whether the individual operands have
>> to be retained equivalent.
> That can certainly be done if necessary.  I'm just not sure it's needed,
> at least not at this time.  If you want to make that change, feel free
> to submit it.
> 
>>
>>>
>>> I've generally found that avoiding const0_rtx is wise.  Instead I
>>> suggest using CONST0_RTX (mode) to get the constant in the proper mode.
>>>
>>
>> Do you mean the (op1 == const0_rtx) test?
> Yes.  It's a fairly minor issue.
> 
> 
>>
>>> Given the tangled mess this code happens to be, could you please do a
>>> bootstrap test on target with and without SELECT_CC_MODE.  x86 would be
>>> a great example of the former, powerpc the latter (use the build farm if
>>> you need to).   For ppc, just bootstrapping would be fine; I don't think
>>> a full regression test is needed, just some verification that we don't
>>> break ppc build should be sufficient.
>>>
>>
>> Bootstrap and test on x86 is completed already, both 32 and 64 bit.
>> Bootstrap on ARM itself is also completed, using a Pandaboard. I'll try
>> doing a ppc build too.
> Thanks. Given that x86 & ARM are both SELECT_CC_MODE targets, I suspect
> they're OK.  I mostly want a sniff test on a target that doesn't define
> SELECT_CC_MODE (ppc for example).
> 
> Thanks,
> jeff
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> 
> iQEcBAEBAgAGBQJNuuDyAAoJEBRtltQi2kC7gMQH/3hANrSFa3MSCTUvNxaiv9sy
> HLoVYgcVXxNCNwYm/uSxZ3titiqyqB7ySrYPEezXCqdaNJeUSk+5v9w23wszyoel
> bITxU6FC1P9wgHCRVNc9NsxpBJeCZleDSPHcrqAdbW8yO6J59qtaaRAlsBjgz11f
> Isg1X+apG0Cy6DZ8knNRDVv9+HyJNLTYCg+90sSQv2Of1obp0b34sq/C2e03+oHz
> gVBkgCvkLazcvYxLrdyCgXfyXvNMbMfSeVclJzpikJZAOAJBCwceYd16wg9ohBZR
> y1g31oZ4teYcLz6c+yWEiWmZpVbeLnZSB9/bkvwP9lrQwkmKyxP33kdYHl/VP1E=
> =ZZwe
> -----END PGP SIGNATURE-----


[-- Attachment #2: combine-20110506.diff --]
[-- Type: text/plain, Size: 16120 bytes --]

Index: combine.c
===================================================================
--- combine.c	(revision 173473)
+++ combine.c	(working copy)
@@ -450,6 +450,7 @@
 				 int);
 static int recog_for_combine (rtx *, rtx, rtx *);
 static rtx gen_lowpart_for_combine (enum machine_mode, rtx);
+static enum rtx_code simplify_compare_const (enum rtx_code, rtx, rtx *);
 static enum rtx_code simplify_comparison (enum rtx_code, rtx *, rtx *);
 static void update_table_tick (rtx);
 static void record_value_for_reg (rtx, rtx, rtx);
@@ -3046,58 +3047,98 @@
 
   if (i1 == 0 && added_sets_2 && GET_CODE (PATTERN (i3)) == SET
       && GET_CODE (SET_SRC (PATTERN (i3))) == COMPARE
-      && XEXP (SET_SRC (PATTERN (i3)), 1) == const0_rtx
+      && CONST_INT_P (XEXP (SET_SRC (PATTERN (i3)), 1))
       && rtx_equal_p (XEXP (SET_SRC (PATTERN (i3)), 0), i2dest))
     {
-#ifdef SELECT_CC_MODE
-      rtx *cc_use;
-      enum machine_mode compare_mode;
-#endif
+      rtx newpat_dest;
+      rtx *cc_use_loc = NULL, cc_use_insn = NULL_RTX;
+      rtx op0 = i2src, op1 = XEXP (SET_SRC (PATTERN (i3)), 1);
+      enum machine_mode compare_mode, orig_compare_mode;
+      enum rtx_code compare_code = UNKNOWN, orig_compare_code = UNKNOWN;
 
       newpat = PATTERN (i3);
-      SUBST (XEXP (SET_SRC (newpat), 0), i2src);
+      newpat_dest = SET_DEST (newpat);
+      compare_mode = orig_compare_mode = GET_MODE (newpat_dest);
 
-      i2_is_used = 1;
-
-#ifdef SELECT_CC_MODE
-      /* See if a COMPARE with the operand we substituted in should be done
-	 with the mode that is currently being used.  If not, do the same
-	 processing we do in `subst' for a SET; namely, if the destination
-	 is used only once, try to replace it with a register of the proper
-	 mode and also replace the COMPARE.  */
       if (undobuf.other_insn == 0
-	  && (cc_use = find_single_use (SET_DEST (newpat), i3,
-					&undobuf.other_insn))
-	  && ((compare_mode = SELECT_CC_MODE (GET_CODE (*cc_use),
-					      i2src, const0_rtx))
-	      != GET_MODE (SET_DEST (newpat))))
+	  && (cc_use_loc = find_single_use (SET_DEST (newpat), i3,
+					    &cc_use_insn)))
 	{
-	  if (can_change_dest_mode (SET_DEST (newpat), added_sets_2,
-				    compare_mode))
+	  compare_code = orig_compare_code = GET_CODE (*cc_use_loc);
+	  compare_code = simplify_compare_const (compare_code,
+						 op0, &op1);
+#ifdef CANONICALIZE_COMPARISON
+	  CANONICALIZE_COMPARISON (compare_code, op0, op1);
+#endif
+	}
+
+      /* Do the rest only if op1 is const0_rtx, which may be the
+	 result of simplification.  */
+      if (op1 == const0_rtx)
+	{
+	  /* If a single use of the CC is found, prepare to modify it
+	     when SELECT_CC_MODE returns a new CC-class mode, or when
+	     the above simplify_compare_const() returned a new comparison
+	     operator.  undobuf.other_insn is assigned the CC use insn
+	     when modifying it.  */
+	  if (cc_use_loc)
 	    {
-	      unsigned int regno = REGNO (SET_DEST (newpat));
-	      rtx new_dest;
-
-	      if (regno < FIRST_PSEUDO_REGISTER)
-		new_dest = gen_rtx_REG (compare_mode, regno);
-	      else
+#ifdef SELECT_CC_MODE
+	      enum machine_mode new_mode
+		= SELECT_CC_MODE (compare_code, op0, op1);
+	      if (new_mode != orig_compare_mode
+		  && can_change_dest_mode (SET_DEST (newpat),
+					   added_sets_2, new_mode))
 		{
-		  SUBST_MODE (regno_reg_rtx[regno], compare_mode);
-		  new_dest = regno_reg_rtx[regno];
+		  unsigned int regno = REGNO (newpat_dest);
+		  compare_mode = new_mode;
+		  if (regno < FIRST_PSEUDO_REGISTER)
+		    newpat_dest = gen_rtx_REG (compare_mode, regno);
+		  else
+		    {
+		      SUBST_MODE (regno_reg_rtx[regno], compare_mode);
+		      newpat_dest = regno_reg_rtx[regno];
+		    }
 		}
+#endif
+	      /* Cases for modifying the CC-using comparison.  */
+	      if (compare_code != orig_compare_code
+		  /* ??? Do we need to verify the zero rtx?  */
+		  && XEXP (*cc_use_loc, 1) == const0_rtx)
+		{
+		  /* Replace cc_use_loc with entire new RTX.  */
+		  SUBST (*cc_use_loc,
+			 gen_rtx_fmt_ee (compare_code, compare_mode,
+					 newpat_dest, const0_rtx));
+		  undobuf.other_insn = cc_use_insn;
+		}
+	      else if (compare_mode != orig_compare_mode)
+		{
+		  /* Just replace the CC reg with a new mode.  */
+		  SUBST (XEXP (*cc_use_loc, 0), newpat_dest);
+		  undobuf.other_insn = cc_use_insn;
+		}	      
+	    }
 
-	      SUBST (SET_DEST (newpat), new_dest);
-	      SUBST (XEXP (*cc_use, 0), new_dest);
-	      SUBST (SET_SRC (newpat),
-		     gen_rtx_COMPARE (compare_mode, i2src, const0_rtx));
-	    }
-	  else
-	    undobuf.other_insn = 0;
+	  /* Now we modify the current newpat:
+	     First, SET_DEST(newpat) is updated if the CC mode has been
+	     altered. For targets without SELECT_CC_MODE, this should be
+	     optimized away.  */
+	  if (compare_mode != orig_compare_mode)
+	    SUBST (SET_DEST (newpat), newpat_dest);
+	  /* This is always done to propagate i2src into newpat.  */
+	  SUBST (SET_SRC (newpat),
+		 gen_rtx_COMPARE (compare_mode, op0, op1));
+	  /* Create new version of i2pat if needed; the below PARALLEL
+	     creation needs this to work correctly.  */
+	  if (! rtx_equal_p (i2src, op0))
+	    i2pat = gen_rtx_SET (VOIDmode, i2dest, op0);
+	  i2_is_used = 1;
 	}
-#endif
     }
-  else
 #endif
+
+  if (i2_is_used == 0)
     {
       /* It is possible that the source of I2 or I1 may be performing
 	 an unneeded operation, such as a ZERO_EXTEND of something
@@ -10811,6 +10852,191 @@
   return gen_rtx_CLOBBER (omode, const0_rtx);
 }
 \f
+/* Try to simplify a comparison between OP0 and a constant OP1,
+   where CODE is the comparison code that will be tested, into a
+   (CODE OP0 const0_rtx) form.
+
+   The result is a possibly different comparison code to use.
+   *POP1 may be updated.  */
+
+static enum rtx_code
+simplify_compare_const (enum rtx_code code, rtx op0, rtx *pop1)
+{
+  enum machine_mode mode = GET_MODE (op0);
+  unsigned int mode_width = GET_MODE_BITSIZE (mode);
+  HOST_WIDE_INT const_op = INTVAL (*pop1);
+
+  /* Get the constant we are comparing against and turn off all bits
+     not on in our mode.  */
+  if (mode != VOIDmode)
+    const_op = trunc_int_for_mode (const_op, mode);
+
+  /* If we are comparing against a constant power of two and the value
+     being compared can only have that single bit nonzero (e.g., it was
+     `and'ed with that bit), we can replace this with a comparison
+     with zero.  */
+  if (const_op
+      && (code == EQ || code == NE || code == GE || code == GEU
+	  || code == LT || code == LTU)
+      && mode_width <= HOST_BITS_PER_WIDE_INT
+      && exact_log2 (const_op) >= 0
+      && nonzero_bits (op0, mode) == (unsigned HOST_WIDE_INT) const_op)
+    {
+      code = (code == EQ || code == GE || code == GEU ? NE : EQ);
+      const_op = 0;
+    }
+
+  /* Similarly, if we are comparing a value known to be either -1 or
+     0 with -1, change it to the opposite comparison against zero.  */
+  if (const_op == -1
+      && (code == EQ || code == NE || code == GT || code == LE
+	  || code == GEU || code == LTU)
+      && num_sign_bit_copies (op0, mode) == mode_width)
+    {
+      code = (code == EQ || code == LE || code == GEU ? NE : EQ);
+      const_op = 0;
+    }
+
+  /* Do some canonicalizations based on the comparison code.  We prefer
+     comparisons against zero and then prefer equality comparisons.
+     If we can reduce the size of a constant, we will do that too.  */
+  switch (code)
+    {
+    case LT:
+      /* < C is equivalent to <= (C - 1) */
+      if (const_op > 0)
+	{
+	  const_op -= 1;
+	  code = LE;
+	  /* ... fall through to LE case below.  */
+	}
+      else
+	break;
+
+    case LE:
+      /* <= C is equivalent to < (C + 1); we do this for C < 0  */
+      if (const_op < 0)
+	{
+	  const_op += 1;
+	  code = LT;
+	}
+
+      /* If we are doing a <= 0 comparison on a value known to have
+	 a zero sign bit, we can replace this with == 0.  */
+      else if (const_op == 0
+	       && mode_width <= HOST_BITS_PER_WIDE_INT
+	       && (nonzero_bits (op0, mode)
+		   & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)))
+	       == 0)
+	code = EQ;
+      break;
+
+    case GE:
+      /* >= C is equivalent to > (C - 1).  */
+      if (const_op > 0)
+	{
+	  const_op -= 1;
+	  code = GT;
+	  /* ... fall through to GT below.  */
+	}
+      else
+	break;
+
+    case GT:
+      /* > C is equivalent to >= (C + 1); we do this for C < 0.  */
+      if (const_op < 0)
+	{
+	  const_op += 1;
+	  code = GE;
+	}
+
+      /* If we are doing a > 0 comparison on a value known to have
+	 a zero sign bit, we can replace this with != 0.  */
+      else if (const_op == 0
+	       && mode_width <= HOST_BITS_PER_WIDE_INT
+	       && (nonzero_bits (op0, mode)
+		   & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)))
+	       == 0)
+	code = NE;
+      break;
+
+    case LTU:
+      /* < C is equivalent to <= (C - 1).  */
+      if (const_op > 0)
+	{
+	  const_op -= 1;
+	  code = LEU;
+	  /* ... fall through ...  */
+	}
+      /* (unsigned) < 0x80000000 is equivalent to >= 0.  */
+      else if (mode_width <= HOST_BITS_PER_WIDE_INT
+	       && (unsigned HOST_WIDE_INT) const_op
+	       == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1))
+	{
+	  const_op = 0;
+	  code = GE;
+	  break;
+	}
+      else
+	break;
+
+    case LEU:
+      /* unsigned <= 0 is equivalent to == 0 */
+      if (const_op == 0)
+	code = EQ;
+      /* (unsigned) <= 0x7fffffff is equivalent to >= 0.  */
+      else if (mode_width <= HOST_BITS_PER_WIDE_INT
+	       && (unsigned HOST_WIDE_INT) const_op
+	       == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1)
+	{
+	  const_op = 0;
+	  code = GE;
+	}
+      break;
+
+    case GEU:
+      /* >= C is equivalent to > (C - 1).  */
+      if (const_op > 1)
+	{
+	  const_op -= 1;
+	  code = GTU;
+	  /* ... fall through ...  */
+	}
+
+      /* (unsigned) >= 0x80000000 is equivalent to < 0.  */
+      else if (mode_width <= HOST_BITS_PER_WIDE_INT
+	       && (unsigned HOST_WIDE_INT) const_op
+	       == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1))
+	{
+	  const_op = 0;
+	  code = LT;
+	  break;
+	}
+      else
+	break;
+
+    case GTU:
+      /* unsigned > 0 is equivalent to != 0 */
+      if (const_op == 0)
+	code = NE;
+      /* (unsigned) > 0x7fffffff is equivalent to < 0.  */
+      else if (mode_width <= HOST_BITS_PER_WIDE_INT
+	       && (unsigned HOST_WIDE_INT) const_op
+	       == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1)
+	{
+	  const_op = 0;
+	  code = LT;
+	}
+      break;
+
+    default:
+      break;
+    }
+
+  *pop1 = GEN_INT (const_op);
+  return code;
+}
+\f
 /* Simplify a comparison between *POP0 and *POP1 where CODE is the
    comparison code that will be tested.
 
@@ -11000,186 +11226,11 @@
 		&& (GET_CODE (op0) == COMPARE || COMPARISON_P (op0))))
 	break;
 
-      /* Get the constant we are comparing against and turn off all bits
-	 not on in our mode.  */
+      /* Try to simplify the compare to constant, possibly changing the
+	 comparison op, and/or changing op1 to zero.  */
+      code = simplify_compare_const (code, op0, &op1);
       const_op = INTVAL (op1);
-      if (mode != VOIDmode)
-	const_op = trunc_int_for_mode (const_op, mode);
-      op1 = GEN_INT (const_op);
 
-      /* If we are comparing against a constant power of two and the value
-	 being compared can only have that single bit nonzero (e.g., it was
-	 `and'ed with that bit), we can replace this with a comparison
-	 with zero.  */
-      if (const_op
-	  && (code == EQ || code == NE || code == GE || code == GEU
-	      || code == LT || code == LTU)
-	  && mode_width <= HOST_BITS_PER_WIDE_INT
-	  && exact_log2 (const_op) >= 0
-	  && nonzero_bits (op0, mode) == (unsigned HOST_WIDE_INT) const_op)
-	{
-	  code = (code == EQ || code == GE || code == GEU ? NE : EQ);
-	  op1 = const0_rtx, const_op = 0;
-	}
-
-      /* Similarly, if we are comparing a value known to be either -1 or
-	 0 with -1, change it to the opposite comparison against zero.  */
-
-      if (const_op == -1
-	  && (code == EQ || code == NE || code == GT || code == LE
-	      || code == GEU || code == LTU)
-	  && num_sign_bit_copies (op0, mode) == mode_width)
-	{
-	  code = (code == EQ || code == LE || code == GEU ? NE : EQ);
-	  op1 = const0_rtx, const_op = 0;
-	}
-
-      /* Do some canonicalizations based on the comparison code.  We prefer
-	 comparisons against zero and then prefer equality comparisons.
-	 If we can reduce the size of a constant, we will do that too.  */
-
-      switch (code)
-	{
-	case LT:
-	  /* < C is equivalent to <= (C - 1) */
-	  if (const_op > 0)
-	    {
-	      const_op -= 1;
-	      op1 = GEN_INT (const_op);
-	      code = LE;
-	      /* ... fall through to LE case below.  */
-	    }
-	  else
-	    break;
-
-	case LE:
-	  /* <= C is equivalent to < (C + 1); we do this for C < 0  */
-	  if (const_op < 0)
-	    {
-	      const_op += 1;
-	      op1 = GEN_INT (const_op);
-	      code = LT;
-	    }
-
-	  /* If we are doing a <= 0 comparison on a value known to have
-	     a zero sign bit, we can replace this with == 0.  */
-	  else if (const_op == 0
-		   && mode_width <= HOST_BITS_PER_WIDE_INT
-		   && (nonzero_bits (op0, mode)
-		       & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)))
-		         == 0)
-	    code = EQ;
-	  break;
-
-	case GE:
-	  /* >= C is equivalent to > (C - 1).  */
-	  if (const_op > 0)
-	    {
-	      const_op -= 1;
-	      op1 = GEN_INT (const_op);
-	      code = GT;
-	      /* ... fall through to GT below.  */
-	    }
-	  else
-	    break;
-
-	case GT:
-	  /* > C is equivalent to >= (C + 1); we do this for C < 0.  */
-	  if (const_op < 0)
-	    {
-	      const_op += 1;
-	      op1 = GEN_INT (const_op);
-	      code = GE;
-	    }
-
-	  /* If we are doing a > 0 comparison on a value known to have
-	     a zero sign bit, we can replace this with != 0.  */
-	  else if (const_op == 0
-		   && mode_width <= HOST_BITS_PER_WIDE_INT
-		   && (nonzero_bits (op0, mode)
-		       & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)))
-		       == 0)
-	    code = NE;
-	  break;
-
-	case LTU:
-	  /* < C is equivalent to <= (C - 1).  */
-	  if (const_op > 0)
-	    {
-	      const_op -= 1;
-	      op1 = GEN_INT (const_op);
-	      code = LEU;
-	      /* ... fall through ...  */
-	    }
-
-	  /* (unsigned) < 0x80000000 is equivalent to >= 0.  */
-	  else if (mode_width <= HOST_BITS_PER_WIDE_INT
-		   && (unsigned HOST_WIDE_INT) const_op
-		      == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1))
-	    {
-	      const_op = 0, op1 = const0_rtx;
-	      code = GE;
-	      break;
-	    }
-	  else
-	    break;
-
-	case LEU:
-	  /* unsigned <= 0 is equivalent to == 0 */
-	  if (const_op == 0)
-	    code = EQ;
-
-	  /* (unsigned) <= 0x7fffffff is equivalent to >= 0.  */
-	  else if (mode_width <= HOST_BITS_PER_WIDE_INT
-		   && (unsigned HOST_WIDE_INT) const_op
-		      == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1)
-	    {
-	      const_op = 0, op1 = const0_rtx;
-	      code = GE;
-	    }
-	  break;
-
-	case GEU:
-	  /* >= C is equivalent to > (C - 1).  */
-	  if (const_op > 1)
-	    {
-	      const_op -= 1;
-	      op1 = GEN_INT (const_op);
-	      code = GTU;
-	      /* ... fall through ...  */
-	    }
-
-	  /* (unsigned) >= 0x80000000 is equivalent to < 0.  */
-	  else if (mode_width <= HOST_BITS_PER_WIDE_INT
-		   && (unsigned HOST_WIDE_INT) const_op
-		      == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1))
-	    {
-	      const_op = 0, op1 = const0_rtx;
-	      code = LT;
-	      break;
-	    }
-	  else
-	    break;
-
-	case GTU:
-	  /* unsigned > 0 is equivalent to != 0 */
-	  if (const_op == 0)
-	    code = NE;
-
-	  /* (unsigned) > 0x7fffffff is equivalent to < 0.  */
-	  else if (mode_width <= HOST_BITS_PER_WIDE_INT
-		   && (unsigned HOST_WIDE_INT) const_op
-		      == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1)
-	    {
-	      const_op = 0, op1 = const0_rtx;
-	      code = LT;
-	    }
-	  break;
-
-	default:
-	  break;
-	}
-
       /* Compute some predicates to simplify code below.  */
 
       equality_comparison_p = (code == EQ || code == NE);

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Paolo Bonzini]

On 04/22/2011 05:21 PM, Chung-Lin Tang wrote:
> Also, instead of testing for XEXP(SET_SRC(PATTERN(i3)),1) == const0_rtx
> at the top, it now allows CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)),
> tries to adjust it by simplify_compare_const() from the last patch, and
> then tests if op1 == const0_rtx. This is a small improvement in some cases.

I'm not sure why it doesn't allow both?

Paolo

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Chung-Lin Tang]

On 2011/5/6 05:57 PM, Paolo Bonzini wrote:
> On 04/22/2011 05:21 PM, Chung-Lin Tang wrote:
>> Also, instead of testing for XEXP(SET_SRC(PATTERN(i3)),1) == const0_rtx
>> at the top, it now allows CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)),
>> tries to adjust it by simplify_compare_const() from the last patch, and
>> then tests if op1 == const0_rtx. This is a small improvement in some
>> cases.
> 
> I'm not sure why it doesn't allow both?
> 
> Paolo

Hi Paolo, I'm not sure I understand your meaning of 'both', but before
this patch, it only tested for == const0_rtx, without any attempt of
other cases.

Now it tests CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)), attempts a
simplification which may change a non-zero constant to const0_rtx, then
test for const0_rtx. Supposedly, the new code should be strictly more
general.

Thanks,
Chung-Lin

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Paolo Bonzini]

On 05/06/2011 12:56 PM, Chung-Lin Tang wrote:
>> >  I'm not sure why it doesn't allow both?
>> >
>> >  Paolo
> Hi Paolo, I'm not sure I understand your meaning of 'both', but before
> this patch, it only tested for == const0_rtx, without any attempt of
> other cases.
>
> Now it tests CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)), attempts a
> simplification which may change a non-zero constant to const0_rtx, then
> test for const0_rtx. Supposedly, the new code should be strictly more
> general.

Uff.  Stupid question is stupid.

Paolo

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/06/11 03:57, Paolo Bonzini wrote:
> On 04/22/2011 05:21 PM, Chung-Lin Tang wrote:
>> Also, instead of testing for XEXP(SET_SRC(PATTERN(i3)),1) == const0_rtx
>> at the top, it now allows CONST_INT_P(XEXP(SET_SRC(PATTERN(i3)),1)),
>> tries to adjust it by simplify_compare_const() from the last patch, and
>> then tests if op1 == const0_rtx. This is a small improvement in some
>> cases.
> 
> I'm not sure why it doesn't allow both?
Part of the purpose of the patch is be more general in the constants
allowed; prior to Chung-Lin's patch only const0_rtx was allowed.
Chung-Lin's patch generalizes the code to allow other constants is
specific cases.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNxA9+AAoJEBRtltQi2kC79lUH/2s2u2HNJMSedW5RFGPhYghX
zIosctPzZ4EkqrH5uvNJMBRxnxu0sBmDcJM5HcoaA5tz/T1aHlsGk6XvPeh+gSJO
wDnFHCUMdmB7hXSB/BcpAC5496DTrZNoyix5qIwIpxPjlaA9n4LoSA+ZiO6nObPH
dZ6UfyCihF+zCukSSQ0qHywJvSVfsQByBYefspS7uy0yFhzm45LHTcIN/j4hC685
lC2lIsBH7ZtMV01tRbr47PGgoey0pwvVeiHf/FcCWA6+Zo2ctfyjzsaE3exg8ms6
zylDHA/9gf2D1oYFn5FmrnHiYt3WGX/75u7bJCCJK1OUKknq6MnexVnfITsovFo=
=7ZnG
-----END PGP SIGNATURE-----

Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine()

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/06/11 03:38, Chung-Lin Tang wrote:
> Hi Jeff,
> I have verified the patch with a native bootstrap + testsuite run on
> powerpc-linux (32-bit), results were clean.
> 
> Attached is a single patch with the 1+2 combine parts together, with
> comments updated. Please check if they feel descriptive enough.
> 
> I haven't updated the CANONICALIZE_COMPARISON stuff, as we discussed it
> doesn't look like absolutely needed right now. As for the const0_rtx
> compare, because the entire case is guarded by a CONST_INT_P, I think it
> should be safe.
> 
> Is this now okay for trunk?

Yes, please install.

Thanks,
jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNxFocAAoJEBRtltQi2kC7IhQH/2P8rOuJloYS4ckDCOhbqBcW
w37R+qlzQztJLKRrI+cxSHl/uUPZ4iJ0NPsZ/WnuMcj2o/eWnU8zERYvky8NGb0g
FnHbhBsRz6cvw0+vEhfBxmZ4i2RKezSZwXquu/Dt4ZZ/Wy4agTMKEQoiimGz2QvR
f8/6JSfkJKLuj/4t/XkoQIzK516ADG1mvvp6CWKR/UoXSnfJKS9eXcmZZ5YMuVpp
NiQ4oXJHGZguH1ecv31l/Eqz6KsJTLsX+3nhriSwfORdlmDGi3IVQZy3vCP02iw8
IFDm5mxH7mUWPrTVaW4wEgMIFdiBIinsC7/mNARO2FLGnkMW++lFLuSWeRc7A9Y=
=darN
-----END PGP SIGNATURE-----