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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to