[PATCH] D61652: [LangRef][Attr] Clarify dereferenceable(_in_scope)

Thu May 16 07:42:56 PDT 2019

jdoerfert added a comment.

In D61652#1504093 <https://reviews.llvm.org/D61652#1504093>, @reames wrote:

> I think the framing of this in terms of scopes is likely to be problematic.  The current definition of dereferenceable is (I think) *stronger* than a simple assertion of the dereferenceability within a particular scope.  As an example, consider:
>  call void @foo(i8* dereferenceable(8) %p)
>  call void @may_trap_but_not_write()
>  %val = load i64, i64 @G
>
> In this example, the current semantic seem to allow the load to be lifted above the second call, even though it is *not* contained within any scope marked dereferenceable.    Now, that's my understanding of the current wording, but I have to admit, I can't find an optimization which actually does that style of reasoning, so maybe I'm mistaken.

The discussion with @sanjoy lead to the change from `scope` to `globally`, maybe that resolves this?

> Switching topics, I have a much more fundamental objection to your patch which comes in two parts.
> 
> Part 1 - By introducing a new attribute and not switching *all* users within the optimizer over, you are implicitly a) causing a bunch of miscompiles and b) setting up a situation where optimizations are likely to be weakened to "fix" bugs.   This to me is a showstopper with the current patch, and on this basis I *strongly object* to this landing without this being addressed.

The situation right now is that LLVM is **broken**. The front-end and middle-end disagree over the definition of `dereferenceable`. That is a problem because we cannot even "fix" because we don't know what part is "right" and what part is "wrong".

The situation after this patch would be the same but we would define which parts of LLVM are "wrong", the front-end or the middle-end (it would be the middle-end). This is a prerequirement to fixing LLVM as it is.

The only alternative (I see) is to define `dereferenceable` as `dereferenceable_globally` and introduce `dereferenceable_here` which would encode what Clang uses `dereferenceable` today for. I'd prefer it this way but at the end of the day, I need some disambiguation to start fixing the existing code.

> Part 2 - It's really not clear to me that your weaker "deref on entry" is strong enough for any useful optimization.  This seems conceptually similar to a "llvm.mark_deref_here" intrinsic which just happens to be placed at the begining of each function.  The problem with such a marker is that you can't (by the intention you've laid out), use it's presence for any path  insensative reasoning.  So for instance, you *can't* use isSafeToSpeculate's context free speculation or LICM's path insensative hoisting/sinking.

You can't use this reasoning, correct. However, we do right now when Clang uses `dereferenceable` for `dereferenceable_on_entry` for lowering C++ references.

I wan this because I assume we have "deref on entry" annotation and an analysis that could reason about "no-free" for functions (D49165 <https://reviews.llvm.org/D49165> integrated into D59918 <https://reviews.llvm.org/D59918>), we could propagate this annotation to pointer uses which would make it useful. Agreed?

> My concern here is purely a generic "is this worth the complexity" one.  If you and others are strongly motivated to have a semantic like this, I will not resist.  In your shoes, I would want to prototype out some key pieces, and would maybe start with an intrinsic before moving straight to an attribute/metadata.

The main motivation is that this is broken right now. If we start to deduce and propagate `dereferenceable` we will expose these inconsistencies. I want to get ahead and make the semantics clear so we can decide which parts have to be fixed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61652