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

Wed Sep 29 11:23:44 PDT 2021

reames created this revision.
reames added reviewers: jdoerfert, nlopes, nhaehnle.
Herald added subscribers: dexonsmith, kerbowa, pengfei, asbirlea, bollu, JDevlieghere, hiraditya, jgravelle-google, sbc100, tpr, jvesely, nemanjai, mcrosier, dschuff.
reames requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added projects: clang, LLVM.

This change shifts all of our mechanisms for specifying dereferenceability to be explicitly point-in-time.  This change covers both argument and return attribute forms of both dereferenceable(N) and dereferenceable_or_null(N) attributes, as well as the parallel family of metadata.

The change in semantics is believed to be backwards compatible - that is, legacy bitcode which expected the previous semantics will not miscompile.  There may be a non-trivial performance (i.e. optimization quality) impact with this change.  Please see detailed discussion later on.

A bit of history for context...

Our existing semantics (as implemented in the optimizer), were primarily scope based.  Once an argument was marked dereferenceable, the underlying memory was assumed to *remain* dereferenceable until the end of the dynamic scope.  It's been pointed out a couple of times that this didn't align with the way clang uses the attribute, and there were even mentions of potential miscompiles.

The tension comes up from the fact that the legacy semantics happen to be really useful for other frontends, and we've been hesitant to give up the optimization power implied.  We don't really have a great mechanism for context-sensitive dereferenceability.

The last iteration of this - from 2019 - eventually evolved into a proposal to split the attributes into two variants - one scoped, one point-in-time.  You can see the history and discussion on D61652 <https://reviews.llvm.org/D61652>, but the proposal never landed.

Earlier this year, I returned to the topic and worked with Johannes (the author of the last attempt) to come up with a proposal which allowed us - we thought - to use the point-in-time semantics while recovering the scoped semantics in all practical cases.  Unfortunately, this attempt failed as it turns out we don't have broad agreement on what some of our function and argument attributes actually mean.  See D99100 <https://reviews.llvm.org/D99100> and linked reviews/RFC for the history.

While this latest effort didn't succeed for *all* cases where the scoped semantics might be useful, it did succeed for one in particular - the GCed language use case which motivated my strong objections to Johannes' earlier proposals.

All of this is to say, we're going back to the original idea of just redefining the attributes to the weaker semantics, and giving up some optimization potential in the progress.

Expected code quality impact

There may be a negative code quality (performance) impact on some workloads.  In particular, there is great uncertainty about the impact on rust code compiled by rustc.  On the C/C++/Clang side, no large impact has been identified to date, but we also have not run extensive benchmarks.  (Reviewer help with that would be very appreciated!)

The risk of negative code quality impacts will be reduced for LTO and languages with larger modules.  We have managed to strengthen the (on by default) inference of the nosync and nofree function attributes, and one of the few cases we got everyone on board with was that an argument to such a function couldn't be freed within it's dynamic scope.  As a result, attribute inference will tend to extend point-in-time semantics to scoped semantics for many common cases, provided we can see the whole call graph.

However, once we inline that function out of existence, we will tend to loose the strong deref fact in the callers scope.  This means there's a pass ordering interaction which is undesirable, but also hard to avoid as we don't yet have a fully context sensitive deref analysis.  We may, in the future, need to consider implementing one.

If you think you might have code quality impact, you can test using the -use-dereferenceable-at-point-semantics=false flag.  Once confirmed, help reproducing and reducing would be very appreciated.

On test changes

I had previously made an effort to go through and add "nofree nosync" to tests where doing so seemed to preserve the spirit of the test.  What's left are cases where a) the semantic change is meaningful, or b) tests which have been updated since my last sweep.  Out of these, the only ones I really haven't investigated are the AMDGPU changes.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110745

Files:
  clang/test/CodeGenOpenCL/builtins-amdgcn.cl
  llvm/docs/LangRef.rst
  llvm/lib/Analysis/BasicAliasAnalysis.cpp
  llvm/lib/IR/Value.cpp
  llvm/test/Analysis/BasicAA/dereferenceable.ll
  llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-load-metadata.ll
  llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-call-return-values.ll
  llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-call.ll
  llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-indirect-call.ll
  llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-sibling-call.ll
  llvm/test/CodeGen/AMDGPU/buffer-intrinsics-mmo-offsets.ll
  llvm/test/CodeGen/AMDGPU/indirect-addressing-term.ll
  llvm/test/CodeGen/AMDGPU/kernel-args.ll
  llvm/test/CodeGen/AMDGPU/legalize-fp-load-invariant.ll
  llvm/test/CodeGen/AMDGPU/store-local.96.ll
  llvm/test/CodeGen/PowerPC/memcmp-mergeexpand.ll
  llvm/test/CodeGen/WebAssembly/reg-stackify.ll
  llvm/test/CodeGen/X86/load-partial.ll
  llvm/test/Transforms/InstCombine/AMDGPU/memcpy-from-constant.ll
  llvm/test/Transforms/LICM/hoist-deref-load.ll
  llvm/test/Transforms/MergeICmps/X86/addressspaces.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D110745.375968.patch
Type: text/x-patch
Size: 150716 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210929/508894d8/attachment-0001.bin>