balazs-benics-sonarsource wrote: > > I was thinking about possible ways to unblock this change. > > If the additional code complexity needs justification, by measuring the > > average impact (to ensure no regression happens in the common cases), and > > by repeating the measurement of the handful of edge-cases where it should > > bring noticeable (10%+) improvement. My estimate of these efforts would be > > a couple of days of work. I probably can't afford to spend this time. > > I don't think that these additional measurements would be worth the effort. > The measurement quality was my chronologically first concern, but it's not > the crux of the matter. > > * If I understand the situation correctly you don't claim that this patch > would provide any benefit that can be detected by users who analyze a > complete real-world software project (and not just individual outlier TUs). > * I find it plausible that this PR indeed improves the runtime of a handful > of edge-cases and I don't suspect that it would increase the overall runtime, > but I don't think that the benefits which are only visible on isolated > outlier entry points (or artificial test cases) justify the additional code > complexity.
I'd say yes to both, while pointing out again that the handful of cases where the improvement can be observed are also real-world code, but rare code. > My personal opinion is that the best decision would be abandoning this PR > (costs outweigh the benefits), but this is an inherently subjective > judgement, so if you are convinced that merging this PR would be better, then > asking other contributors is the right way forward (and I won't block the > decision if others agree with it). Thank you for your flexibility. I still believe that obeying max symbol complexity on its own brings value even without knowing that sometimes it also eliminates long running edge cases. I believe it justifies for the added complexity, but I'm very open about refining the patch in other directions that would also ensure max symbol complexity. /cc @Xazax-hun @haoNoQ for second opinion. https://github.com/llvm/llvm-project/pull/144327 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
