llvm dev - Apr 2017 - Dereferenceable load semantics & LICM

2017-04-05 18:30 GMT+02:00 Sanjoy Das <sanjoy at playingwithpointers.com>:
> Hi Piotr,
>
> On Mon, Apr 3, 2017 at 2:01 PM, Piotr Padlewski
> <piotr.padlewski at gmail.com> wrote:
> >> I don't see any way to model it right now in LLVM, but I see a
one
> simple
> >> solution:
> >>  add extra information to the metadata nodes indicating that this
> property
> >> is non-local:
> >>
> >> %0 = load... %p, !invariant.group !1, !dereferenceable !2
> >> %1 = load ... %0, !invariant.load !0, !dereferenceable !2
> >>
> >> !0 = !{!"GlobalProperty"}
> >> !1 = !{!"MyType", !"GlobalProperty"}
> >> !2 = !{i64 8, !"GlobalProperty}
> >>
> >> With that we would strip only the metadata not containing this
> >> information.
> >>
> >> For devirtualization it would make sense with invariant.load,
> >> invariant.group and dereferenceable.
> >>
> >> What do you think about this idea?
>
> I'm sorry for being *this* annoying, but I'm not on board with
this.  :)
>
> I think this will run into the same dead-code-can-affect-behavior
> issue discussed in https://reviews.llvm.org/D20116.
>
> Specifically, if your program is:
>
> if (false) {
>   ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8,
!"GlobalProperty}
>   // ptr is not actually dereferenceable, even the load above has UB
>   // (since the metadata is "wrong"), but it is never executed so
all is
> well.
>   int val = *ptr;
> }
>
> then you could transform it to
>
> ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8,
!"GlobalProperty}
> // ptr is not actually dereferenceable
> int val = *ptr;
> if (false) {
> }
>
> and you'd have gone from a program with no UB to one with UB.
>
> -- Sanjoy
>
I disagree, I find it different than the patch you mentioned. We don't have
any problems with code like this:

ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8}
if (false) {

  // ptr is not actually dereferenceable, even the load above has UB
  // (since the metadata is "wrong"), but it is never executed so all
is
well.
  int val = *ptr;
}

because we rely on the information provided by someone else that ptr is
dereferenceable, so we can transform it to

ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8}
// ptr is not actually dereferenceable
int val = *ptr;
if (false) {
}

And this is exactly how we treat any other UB.
It is also important to mention that in order to perform optimization you
have mentioned we need to know that %ptrptr is also dereferenceable.
The vptr and vtable properties are non local, so I don't see how
hoisting vptr load could be dangerous:

void foo(A& a) {
  if (false)
    a.foo();
}

will be:

void foo(A* dereferenceable(8) %a) {
  if (false)
    %vptr = load %a, !dereferenceable !{VtableSize, "GlobalProperty"},
!invariant.group !0
    %vfunction = load %vptr, !invariant.load !0
    call %vfunction();
}

and after hoisting:

void foo(A* dereferenceable(8) %a) {
  %vptr = load %a, !dereferenceable !{VtableSize, "GlobalProperty"},
!invariant.group !{"GlobalProperty"}
  %vfunction = load %vptr, !invariant.load !{"GlobalProperty"}
  if (false)
    call %vfunction();
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170406/ed368791/attachment.html>

Hi Piotr,

On April 6, 2017 at 2:36:53 AM, Piotr Padlewski
(piotr.padlewski at gmail.com) wrote:
> I disagree, I find it different than the patch you mentioned. We don't
have
> any problems with code like this:
>
> ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8}
> if (false) {
>
> // ptr is not actually dereferenceable, even the load above has UB
> // (since the metadata is "wrong"), but it is never executed so
all is
> well.
> int val = *ptr;
> }
I was not talking about code like that.  The problem is code like this:

if (false) {
  ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8, !"GlobalProperty}
  // ptr is not actually dereferenceable, even the load above has UB
  // (since the metadata is "wrong"), but it is never executed so all
is well.
  int val = *ptr;
}

I did not mention this earlier, but I've assumed that %ptrptr itself
is dereferenceable, which means you can hoist the load of ptr.  Since
because of !"GlobalProperty" you don't strip the !dereferenceable,
you'll also be able to hoist the load of val, which would segfault
because ptr was not dereferenceable.

That is, with the !"GlobalProperty" bit in the picture, it is possible
to make "if (false) { X }" introduce UB in a program for certain
values of X.

-- Sanjoy

2017-04-06 17:57 GMT+02:00 Sanjoy Das <sanjoy at playingwithpointers.com>:
> Hi Piotr,
>
> On April 6, 2017 at 2:36:53 AM, Piotr Padlewski
> (piotr.padlewski at gmail.com) wrote:
> > I disagree, I find it different than the patch you mentioned. We
don't
> have
> > any problems with code like this:
> >
> > ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8}
> > if (false) {
> >
> > // ptr is not actually dereferenceable, even the load above has UB
> > // (since the metadata is "wrong"), but it is never executed
so all is
> > well.
> > int val = *ptr;
> > }
>
> I was not talking about code like that.  The problem is code like this:
>
> if (false) {
>   ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8,
!"GlobalProperty}
>   // ptr is not actually dereferenceable, even the load above has UB
>   // (since the metadata is "wrong"), but it is never executed so
all is
> well.
>   int val = *ptr;
> }
>
> I did not mention this earlier, but I've assumed that %ptrptr itself
> is dereferenceable, which means you can hoist the load of ptr.  Since
> because of !"GlobalProperty" you don't strip the
!dereferenceable,
> you'll also be able to hoist the load of val, which would segfault
> because ptr was not dereferenceable.
>
> That is, with the !"GlobalProperty" bit in the picture, it is
possible
> to make "if (false) { X }" introduce UB in a program for certain
> values of X.
>
> Hi Sanjoy,
My point is that this it is exactly the same way as normal !dereferenceable
introduces UB.

 ptr = load i8*, i8** %ptrptr, !dereferenceable !{i64 8}
 if (false) {
  int val = *ptr;
  }

If frontend says that something is dereferenceable, which is not actually
dereferenceable,  then it is UB and everything can happen - like the
execution of dead instruction.
This is exactly the same with the global properties - we are giving a
guarantee that pointer it is dereferenceable even if we would hoist or sink
it, and if it is not true then it is UB.

I don't see why UB with normal dereferenceable is acceptable, but having
this property globally is not.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170406/dad3e9a9/attachment.html>