NagyDonat wrote: With the two recent commits I think I addressed all review comments except for the "on-by-default config option" vs "unconditionally active logic" question.
In this area (which we discussed in inline comments) I'm still leaning towards the currently implemented `legacy-inlining-prevention` config option (which is enabled by default), because this would provide very minor advantages in several areas: - The average slowdown caused by disabling this workaround seems to be around 10-15% on larger projects, which is painful, but not completely prohibitive (and I guess that it could be offset if the user e.g. reduces `max-nodes` by 20%). - If somebody happens to run into false negatives (or invalidation-based false positives) which are caused by this inlining prevention, we can suggest `legacy-inlining-prevention=false` as a workaround. - I hope that I will be able to develop other heuristics that prevent the inlining of complex functions based on more "natural" and robust conditions -- when those are available, some users may want to disable this and enable those instead. (Or, if they are proven to be better and we change the defaults, this option should remain as a disabled-by-default fallback for those who might want return to the earlier behavior.) - During the development and testing of those alternative heuristics it would be nice to have this config option to enable/disable this workaround. - Last, but perhaps not least, having a named config option will make it easier to speak about this code fragment (instead of "that hack where we disable inlining of a function if we would assume entering the third iteration in a loop"). If we keep the config option, we can also think about renaming it. I agree that `legacy` is a bit unfortunate in the name of an option that is on-by-default (at least for now), but I don't have a better idea. (Feel free to suggest names if you have!) @balazs-benics-sonarsource @Xazax-hun WDYT about this area? Do you have any other question/remark? https://github.com/llvm/llvm-project/pull/136720 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
