[PATCH] D61652: [LangRef][Attr] Clarify dereferenceable(_in_scope)
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 15 20:12:00 PDT 2019
sanjoy accepted this revision.
sanjoy added inline comments.
This revision is now accepted and ready to land.
================
Comment at: llvm/docs/LangRef.rst:1164
+
+``dereferenceable_in_scope(<n>)``
+ This indicates that the annotated pointer value has the
----------------
jdoerfert wrote:
> sanjoy wrote:
> > jdoerfert wrote:
> > > sanjoy wrote:
> > > > I'm not sure if this is a good semantic property. Consider (the CFG form of):
> > > >
> > > > ```
> > > > if (cond) {
> > > > a = dereferenceable_in_scope(4) ...
> > > > } else {
> > > > ...
> > > > }
> > > > side_effect_that_frees_a()
> > > > ```
> > > >
> > > > I think this program above is well defined because the definition of `a` does not dominate `side_effect_that_frees_a()`. But that means we can't tail duplicate the program into:
> > > >
> > > > ```
> > > > if (cond) {
> > > > a = dereferenceable_in_scope(4) ...
> > > > side_effect_that_frees_a()
> > > > } else {
> > > > ...
> > > > side_effect_that_frees_a()
> > > > }
> > > > ```
> > > >
> > > > Same applies for inlining. Any transform that can extend the dominance region of some value needs to be audited.
> > > >
> > > > What is the use case for this attribute? Can we do something more targeted for this use case?
> > > > What is the use case for this attribute? Can we do something more targeted for this use case?
> > >
> > > My "use case" is basically the clang interpretation of (current) `derferenceable` which I tried to replicate with the new `derferenceable` definition. There are users of (current) `derferenceable` that interpret it as `derferenceable_in_scope` and they should have a way to do so.
> > >
> > > > I think this program above is well defined because the definition of a does not dominate
> > > > Same applies for inlining. Any transform that can extend the dominance region of some value needs to be audited.
> > >
> > > I agree that there is a problem with the current wording wrt. inlining. I'll try to fix that.
> > > Maybe @reames can help me define what they want/need.
> > >
> > > For the record, the example above is not sufficient to expose the inlining problem though. Above, a was "accessible" at the location where a was freed, so the `dereferenceable_in_scope` was violated already. Thinking about it, even if we inline, SSA dominance property should be combined with reachability to a user to give you what you want.
> > > I agree that there is a problem with the current wording wrt. inlining. I'll try to fix that. Maybe @reames can help me define what they want/need.
> >
> > Maybe we need something stronger that says that a pointer is always `dereferenceable(n)`, like a global variable?
> >
> > > Thinking about it, even if we inline, SSA dominance property should be combined with reachability to a user to give you what you want.
> >
> > I was thinking about something like:
> >
> > ```
> > void f() {
> > a = dereferenceable_in_scope(4) ...
> > }
> >
> > void main() {
> > f();
> > free_a();
> > }
> >
> > ```
> >
> > I think this is well defined according to the current wording since the `free_a` does not lie within the scope of `a`? If yes, then inlining becomes illegal.
> What do you think about `dereferenceable_globally(<n>)`, which would be the wording without the "in its scope (asdefined by the SSA dominance property)"?
>
> So:
>
>
> This indicates that the annotated pointer value has the
> ``dereferenceable(<n>)`` property *at any program point*. Thus, unlike pointer values
> annotated with ``dereferenceable(<n>)``, ``dereferenceable_in_scope(<n>)``
> pointer values can never lose the ``dereferenceable(<n>)``.
>
I think this is implied, but how about "the annotated pointer value has the
``dereferenceable(<n>)`` starting from the definition of the value to the termination of the program."?
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