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

[Philip Reames via Phabricator via llvm-commits]

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

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.

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.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61652