On Mon, Dec 9, 2013 at 6:15 AM, Bin.Cheng <amker.ch...@gmail.com> wrote: > On Mon, Dec 9, 2013 at 12:00 PM, Jeff Law <l...@redhat.com> wrote: >> On 12/06/13 19:44, Bin.Cheng wrote: >>>> >>>> Right. Based on reading the archives, it looks like this stuff is/was >>>> generated by PRE. I also suspect jump threading can create them. There >>>> was >>>> talk of throttling PRE to leave things in a form that the IV analysis >>>> could >>>> more easily digest, but I'm not sure if that was ever completed. >>> >>> >>> Also could just because it is coded in that way, just as the case I >>> added in patch. I found real examples from ggc-page.c in GCC. >>> But it's always good to cleanup input of an optimization, I presume >>> that's why Richard tried to move IVOPT later before. >> >> Certainly. That possibility was mentioned as well in either 41488 or >> elsewhere in my research. >> >> >>>> >>>> I assume tree_to_aff_combination_expand is relatively expensive, thus the >>>> two approaches, one which avoids tree_to_aff_combination_expand. >>> >>> Considering the dump of case in the patch: >> >> [ snip ] >> So it's also a case using the affine stuff gets you get a simpler interface >> to express the value in terms you may be able match. > Yeah. > >> >> >>> >>> Another question, is it acceptable to add an parameter for >>> tree_to_aff_combination_expand to limit the number of recursive call >>> for it? Thus we don't need to expand to leaf node every time. >> >> If there's a compelling reason to limit the recursion, then I'd think such a >> parameter would be reasonable. > Not for now. Just come up this idea given that some recent patches > also use the interface to get back trace information and it's > expensive. Maybe Richard have some comment here too.
IMHO the natural limit should be MAX_AFF_ELTS - it shouldn't ever try to fill up more slots than that, as if the aff_combination_add at the end will end up populating comb->rest the whole thing was pretty pointless. Also memory management of the cache it uses can be improved by using an obstack alongside the pointer-map to avoid calling malloc/free per element. Also the pointer-map should rather use the SSA name it expands as base which would make formulating the cache as a lattice possible. Richard. >> >> >> >>> >>>> >>>> >>>> In add_old_iv_candidates, is it the case that the only time >>>> SSA_NAME_DEF_STMT (def) is another PHI node is when it's one of these >>>> affine >>> >>> >>> I suppose. Actually IVOPT make an assert on IP_ORIGINAL candidates in >>> function rewrite_use_nonlinear_expr, like: >> >> Well, the reason I ask is after your patch we'd be ignoring anything where >> SSA_NAME_DEF_STMT (def) is a PHI node and not a PEELED_CHREC. I want to >> make sure there aren't other expected cases where SSA_NAME_DEF_STMT (def) is >> a PHI that we'd want to call add_candidate_1 for. >> >> It sounds like there aren't other cases you'd expect to see here where >> SSA_NAME_DEF_STMT (defFFF) is a PHI. > Yes. > >> >> >>> IVOPT adds original cand and tries to keep it the original IV is >>> because it does have an updating statement. For IVs picked up by this >>> patch, it doesn't have stepping statement, so no need/way to leaving >>> it untouched. It will be represented by candidates for IVs from which >>> it is peeled. >> >> Understood. Thanks for clarifying. The patch is fine for the trunk. > Thanks very much for reviewing. Since Sebastian hasn't added any > comments, I will change the patch as suggested by Richard before, then > retest it, and check in the new version if it's ok. > > Thanks, > bin > >> >> jeff > > > > -- > Best Regards.
