Message ID | 4DB19D03.8050606@codesourcery.com |
---|---|
State | New |
Headers | show |
-----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-----
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
-----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-----
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
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
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
-----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-----
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