[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

Sat Nov 13 08:55:55 PST 2021

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