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
