[PATCH] D18738: Add new !unconditionally_dereferenceable load instruction metadata

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added a comment.

In https://reviews.llvm.org/D18738#722628, @sanjoy wrote:

> In https://reviews.llvm.org/D18738#721295, @Prazek wrote:
>
> > I think we already have the same type of optimization and it is fine. Consider:
> >
> >   i8 foo(i8* dereferenceable(8) %p) {
> >     %v = load i8, i8* %p
> >     ret i8 %v
> >   }
> >   
> >
> > Now if you have code like:
> >
> >   void main(i8* %p) {
> >   
> >   }
> >    
> >
> > you can argue that you could transform it into:
> >
> >   void main(i8* %p) {
> >     if (false) {
> >        call foo(i8* %p)
> >     }
> >   }
>
>
> Yes, that needs to be legal.
>
> > resulting in possibility of hoisting the load of %p from foo based on dereferenceable(8) attribute.
>
> How would you justify that?  `%p` is `dereferenceable(8)` //iff// `foo(%p)` is executed.  In the above program `foo(%p)` is not executed, so `%p` is not dereferneceable (outside the `if (false)` block, inside the `if (false)` block anything is "valid" since it is dead code).
>
> > I would argue that this transformation is invalid, because you are introducing new information about the %p that wasn't there.
>
> I don't think you've introduced any new information that is valid outside the `if (false)` block.


You are right, the dereferenceable does not propagate outside of BB it is being executed.

I think I see your point now. There is probably no way of specifying this metadata so it would not affect the program behavior if it is introduced in dead block.

I would consider transformation like:

  int main() {
  }

to

  int main() {
    if (false) { something with global dereferenceable }
  }

Because we are lying that something is dereferenceable everywhere, but it is actually only in this BB.

The problem is that this property could be introduced with a function call like

  int main() {
    if (false) call();
  }

and because the attribute is not transitive in any way, we don't know if such transformation is legal or not.

On the other hand, thinking about what could go wrong if we would add this to vtable loads etc, I can't find anything that would caue our program to behave in different way, because the
property will be always valid (unless LLVM would introduce the code with !global_dereferenceable from thin air) - even if we would not mark every vtable load with it, 
doing the transformations that you mentioned would still be valid, like:

  int main() {
    if (false) { // introduced by a speculative call or something
      load %p, !global_dereferenceable
    }
    else {
      load %p
    }
  }

Figuring out that the second load might have !global_dereferenceable is legit

So in the summary, if this feature would be used to model some higher language feature, that is valid everywhere in the program like:

- the fact that loaded vtable is dereferenceable for all slots, or
- the fact that vtable is constant

then if llvm will do sane transformation (as long as it does not add this metadata from the air), then even inlining function all inside the dead block should be valid.


Repository:
  rL LLVM

https://reviews.llvm.org/D18738