nikic added a subscriber: aeubanks. nikic added a comment. In D110745#3128719 <https://reviews.llvm.org/D110745#3128719>, @reames wrote:
> @nikic ping on previous question. It's been a month, and this has been > LGTMed. Without response, I plan to land this. Sorry, I did do some measurements but forgot to report back. The only run-time workload I can easily measure are rustc check builds, where I observed regressions ranging between -0.6% and +4.8% across different projects (the -0.6% "regressions" indicate an improvement, i.e. that some deref-based optimization happens to be non-profitable). I'm not sure that helps you much apart from saying that yes indeed, this does have some impact on rust code. It's not catastrophic (though caveat emptor: impact on "benchmarky" code may well be higher), but it's also avoidable. > In D110745#3038848 <https://reviews.llvm.org/D110745#3038848>, @xbolva00 > wrote: > >> This really needs to be properly benchmarked. > > This has been benchmarked on every workload I care about, and shows no > interesting regressions. Unfortunately, those are all non-public Java > workloads, I think something important to mention here is that the Java (i.e. GC'd language) case is also the only where we don't really expect a regression, because it has good modeling under the proposed patch: GC'd pointers can't be freed during the optimization pipeline (until statepoint lowering), so they're simply not affected by this change. For that reason I don't think the fact that Java workloads don't see regressions really tells us anything about how this would behave for other frontends, which are mostly not GC'd. > On the C/C++ side, I don't have a ready environment in which to run anything > representative. From the semantic change, I wouldn't expect C++ to show much > difference, and besides, this is fixing a long standing fairly major > correctness issue. If you have particular suites you care about, please run > them and share results. Maybe @asbirlea or @aeubanks can run some benchmarks? I would expect regressions for C++, because `nofree` inference is very limited (e.g. won't be inferred for pretty much any code using growing STL containers). Though at least in the C++ case regressions may be somewhat justified, in that this fixes a correctness issue in some cases. > At this point, I strongly lean towards committing and letting regressions be > reported. We might revert, or we might simply fix forward depending on what > comes up I'm not sure what you're expecting from that. At least as far as Rust is concerned, the problem here seems pretty well-understood to me: You are dropping (without replacement) the ability to specify that an argument is dereferenceable within a function. I'm perfectly happy with the change in default behavior, all I want is a way to get the old one back. I don't think that having an example of that in "real" code is going to add any useful information. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110745/new/ https://reviews.llvm.org/D110745 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
