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

Wed Dec 1 09:45:22 PST 2021

reames abandoned this revision.
reames added a comment.

I'm stopping work on this.  This has already exceeded the amount of work which is worthwhile for me, and it seems there is yet more work needed.

On @nikic's prompting, I finally went ahead and got the test-suite setup and tested a clang version with and without the flag thrown.  Unfortunately, the results were not pretty.  We have significant regressions in some of the memset/memcpy tests.   I did not bother to dig into why in detail, but I suspect the lack of context sensitivity is biting us.

Between the measured regressions on both C++ and rust, I don't think this can go in.

At this point, I've done everything I reasonable can to drive this to conclusion.  My actual motivation for this was a purely defensive effort to avoid regressing Java performance when this someday got fixed, and to make a good faith effort to justify my objections to Johannes' original patches.  That is complete.

Frankly, I think it's incredibly unfortunate that clang has an active miscompile, and no one seems motivated to fix that after *years* of it being there.  However, I have no commercial interest in clang, and the amount of work that seems to be remaining is well beyond anything I'm willing to do on a volunteer basis.

Let me summarize some ideas on future direction for the next poor person who stumbles into this rats nest.

The approach taken in this round of trying to infer scoped dereferenceability from existing attributes and to strengthen the inference of such to cover practical cases has been partially successful, but I no longer believe can be pushed across the finish line.  The problem here is not technical, but political.  We appear to have unresolved disagreements about the semantics of attributes, and the review process towards resolving those disagreements touch on many otherwise disjoint parts of the project.  I would definitely not advise moving further in this direction unless you greatly enjoy herding cats.

We could implement a contextual dereferenceability analysis.  This is useful to have no matter what, but requires extending the current must-execute logic and finding ways to efficiently make that information available cheaply through much of the pass pipeline.  I have some ideas on that, and if someone wants to brainstorm this, feel free to reach out.  However, I think it needs to be said that its unclear if a "perfect" version of this analysis is enough to recover the scoped facts in all cases.  This is a fairly speculative approach, and it might not be enough.

The approach taken in D61652 <https://reviews.llvm.org/D61652> of splitting the dereferenceability attribute into two is a bit ugly.  The objection to this approach in this round was mostly driven by the observation that the "alloc-size" attribute had the same semantic split around whether the implied dereferenceability was scoped or not.  The good news is that the work done in this round was enough to cover performance regressions from the "alloc-size" version, and at this point, the only checked in code for "alloc-size" uses the non-globally dereferenceable semantics.  (We had to because it was actively miscompiling otherwise.)

Personally, if I was motivated to continue working on this, I'd probably resurrect D61652 <https://reviews.llvm.org/D61652> and call it a day.

@nikic - Since you now have the sole remaining frontend for which dropping global deref is a performance regression without also being a correctness fix, any chance you're interested in driving this further?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110745/new/

https://reviews.llvm.org/D110745