[PATCH] D61652: [LangRef][Attr] Clarify dereferenceable(_in_scope)
Philip Reames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 15 20:33:42 PDT 2019
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
More information about the llvm-commits
mailing list