> Thanks for your review. > > > In general the patch looks good to me, but I would like Martin Jambor to > > comment on the ipa-prop/cp interfaces. However... > > > +@item ipa-cp-max-recursion-depth > > +Maximum depth of recursive cloning for self-recursive function. > > + > > > ... I believe we will need more careful cost model for this. I think > > we want to limit the overall growth for all the clones and also probably > > enable this only when ipa-predicates things the individual clones will > > actualy be faster by some non-trivial percentage. For recursive inliner > > we have: > > Cost model used by self-recursive cloning is mainly based on existing stuffs > in ipa-cp cloning, size growth and time benefit are considered. But since > recursive cloning is a more aggressive cloning, we will actually have another > problem, which is opposite to your concern. By default, current parameter > set used to control ipa-cp and recursive-inliner gives priority to code size, > not completely for performance. This makes ipa-cp behave somewhat
Yes, for a while the cost model is quite off. On Firefox it does just few clonings where code size increases so it desprately needs retuning. But since rescursive cloning is quite a different case from normal one, perhaps having independent set of limits would help in particular ... > conservatively, and as a result, it can not trigger expected recursive cloning > for the case in SPEC2017.exchange2 with default setting, blocked by both > ipa-cp-eval-threshold and ipcp-unit-growth. The former is due to improper > static profile estimation, and the latter is conflicted to aggressiveness of > recursive cloning. Thus, we have to explicitly lower the threshold and raise > percentage of unit-growth. > > We might not reach the destination in one leap. This patch is just first step > to enable recursive function versioning. And next, we still need further > elaborate tuning on this. > > > --param max-inline-recursive-depth which has similar meaning to your > > parameter > > (so perhaps similar name would be good) > > --param min-inline-recursive-probability > > which requires the inlining to happen only across edges which are > > known to be taken with reasonable chance > > --param max-inline-insns-recursive > > which specifies overall size after all the recursive inlining > > > Those parameters are not parituclarly well tought out or tested, but > > they may be good start. > > > Do you have some data on code size/performance effects of this change? > For spec2017, no obvious code size and performance change with default > setting. > Specifically, for exchange2, with ipa-cp-eval-threshold=1 and > ipcp-unit-growth=80, > performance +31%, size +7%, on aarch64. ... it will help here since ipa-cp-eval-threshold value needed are quite off of what we need to do. I wonder about the 80% of unit growth which is also more than we can enable by default. How it comes the overal size change is only 7%? Honza > > Feng
