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

[Philip Reames via Phabricator via cfe-commits]

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  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  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

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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a subscriber: aeubanks.
nikic added a comment.

In 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 , @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

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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.
Herald added a subscriber: jeroen.dobbelaere.

@nikic ping on previous question.  It's been a month, and this has been LGTMed. 
 Without response, I plan to land this.

In 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,

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.

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


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

This really needs to be properly benchmarked.




Comment at: llvm/test/Transforms/InstCombine/AMDGPU/memcpy-from-constant.ll:9
 
 ; Simple memcpy to alloca from constant address space argument.
 define i8 @memcpy_constant_arg_ptr_to_alloca([32 x i8] addrspace(4)* noalias 
readonly align 4 dereferenceable(32) %arg, i32 %idx) {

No longer happens..



Comment at: llvm/test/Transforms/LICM/hoist-deref-load.ll:419
 ; and we want to hoist the load of %c out of the loop. This can be done only
 ; because the dereferenceable meatdata on the c = *cptr load.
 define void @test7(i32* noalias %a, i32* %b, i32** %cptr, i32 %n) #0 {

Regression in that C code?


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

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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D110745#3035975 , @nikic wrote:

> Sorry, but the fact that there is still no way to opt-in to the old behavior 
> is still a blocker from my side. If we can't use `dereferenceable + nofree` 
> arguments for that purpose, then we need to provide a different way to do 
> that. Like `dereferenceable + really_nofree`. It looks like the current 
> implementation doesn't even accept the `dereferenceable + nofree + noalias` 
> case originally proposed (which is pretty bad from a design perspective, but 
> would at least work fairly well for rustc in practice). I don't think that 
> our current analysis capabilities are sufficient to land this change at this 
> time.

@nikic Do you have any specific examples of where this causes a workload to 
regress?  At this point, I really need something specific as opposed to a 
general concern.  We're at the point where perfection is very much the enemy of 
the good here.  As noted, I've already spent a lot of time trying to minimize 
impact.


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

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

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

Sorry, but the fact that there is still no way to opt-in to the old behavior is 
still a blocker from my side. If we can't use `dereferenceable + nofree` 
arguments for that purpose, then we need to provide a different way to do that. 
Like `dereferenceable + really_nofree`. It looks like the current 
implementation doesn't even accept the `dereferenceable + nofree + noalias` 
case originally proposed (which is pretty bad from a design perspective, but 
would at least work fairly well for rustc in practice). I don't think that our 
current analysis capabilities are sufficient to land this change at this time.


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

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

[Nuno Lopes via Phabricator via cfe-commits]

nlopes accepted this revision.
nlopes added a comment.
This revision is now accepted and ready to land.

LGTM. We have discussed this before. It's important to fix the reference types 
in C++.




Comment at: llvm/test/Analysis/BasicAA/dereferenceable.ll:66
 ; CHECK: Function: local_and_deref_ret_2: 2 pointers, 2 call sites
-; CHECK-NEXT:  NoAlias:i32* %obj, i32* %ret
+; CHECK-NEXT:  MayAlias:   i32* %obj, i32* %ret
 bb:

this one is unfortunate.. They can't alias because objsize(%obj) < 
dereferenceable size of %ret. This case should be trivial to catch.


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