diff mbox

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

Message ID 4DB19D03.8050606@codesourcery.com
State New
Headers show

Commit Message

Chung-Lin Tang April 22, 2011, 3:21 p.m. UTC 
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

Comments

Jeff Law April 25, 2011, 6:08 p.m. UTC | #1 
-----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-----
Chung-Lin Tang April 26, 2011, 11:44 a.m. UTC | #2 
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
Jeff Law April 29, 2011, 4:01 p.m. UTC | #3 
-----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-----
Paolo Bonzini May 6, 2011, 9:57 a.m. UTC | #4 
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
Chung-Lin Tang May 6, 2011, 10:56 a.m. UTC | #5 
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
Paolo Bonzini May 6, 2011, 11:51 a.m. UTC | #6 
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
Jeff Law May 6, 2011, 3:10 p.m. UTC | #7 
-----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-----
diff mbox

Patch

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