[LLVMdev] GVN incorrectly handling readnone parameter attribute?

Fri May 23 11:56:34 PDT 2014

On 23 May 2014 09:42, Robert Lougher <rob.lougher at gmail.com> wrote:

> Hi Nick,
>
> Thanks for replying.  Bug filed:
> http://llvm.org/bugs/show_bug.cgi?id=19842

Thank you!

Strangely enough, my first conclusion was that %p was being marked
> readnone incorrectly as it wasn't handling the copy via @get_addr.
>

Sorry -- saying %p alone is ambiguous because there's the %p parameter
of @get_pntr
and the %p parameter of @store. It is correct to mark %p readnone in
@get_pntr. From function entrance to exit, it does not write to the pointer
passed in. It is incorrect to mark %p readnone in @store, as it receives a
pointer then writes to it. Supposing the @get_pntr loaded a pointer from
some global variable then wrote through it, then @get_pntr could still mark
its %p as being readnone, since the write was not through %p particularly,
it just happens to be through a pointer that aliased it.

Somebody (not here) then pointed out the readnone definition in the
> LangRef (specifically the "even though it may read or write
> the memory that the pointer points to if accessed through other
> pointers").  I decided the copy fell within this definition and
> changed my mind and decided GVN was at fault.  How should the
> definition be interpreted?
>

Consider:

define void @example(i32* %p, i32* %q) {
  store i32 10, i32* %q
  %A = icmp ult i32* %p, %q
  call void @use(i1 %A) readnone
  ret void
}

@example does not modify the memory pointed to by %p, unless it does it
through a different pointer, hence it may mark that parameter readnone. A
caller may choose to pass in %p == %q in which case the memory pointed-to
may change, but @example still lives up to its contract.

The real-world implications of this are that callers may use this
information to optimize themselves, combined with other information such as
knowing that %p != %q, or better, that %p is a local alloca or freshly
malloc'd memory and the pointer %p has not yet escaped (aka. been captured).

If we read the language as permitting the function to make copies of the
pointers the this code would be valid:

define void @writes_over_everything(i32* readnone %p, i32* readnone %q) {
  %P = bitcast i32* %p to i32*
  %Q = bitcast i32* %p to i32*
  store i32 10, i32* %P
  store i32 10, i32* %Q
  ret void
}

and that's not useful for optimization.

Nick

> On 23 May 2014 04:22, Nick Lewycky <nlewycky at google.com> wrote:
> > Confirmed, this is a bug. This
> >
> > define i32* @get_pntr(i32* %p) nounwind uwtable {
> > entry:
> >   ret i32* %p
> > }
> > define void @store(i32* %p) noinline nounwind uwtable {
> > entry:
> >   %call = call i32* @get_pntr(i32* %p)
> >   store i32 10, i32* %call, align 4
> >   ret void
> > }
> >
> > run through opt -functionattrs gets a 'readnone' on @store's %p. That's
> > wrong, it clearly stores to it. The bug is due to incorrectly handling
> the
> > copy of the pointer made by @get_pntr, because @get_pntr itself is marked
> > 'readnone'.
> >
> > Please file a bug.
> >
> > Nick
> >
> >
> >
> > On 22 May 2014 08:15, Robert Lougher <rob.lougher at gmail.com> wrote:
> >>
> >> Hi Philip,
> >>
> >> Thanks for the reply.
> >>
> >> On 22 May 2014 01:31, Philip Reames <listmail at philipreames.com> wrote:
> >> >
> >> > On 05/21/2014 02:52 PM, Robert Lougher wrote:
> >> >>
> >> >> On 21 May 2014 21:40, Robert Lougher <rob.lougher at gmail.com> wrote:
> >> >>>
> >> >>> define i32* @get_pntr(i32* readnone %p) {
> >> >>> entry:
> >> >>>    ret i32* %p
> >> >>> }
> >> >>>
> >> >>> define void @store(i32* nocapture readnone %p) {
> >> >>> entry:
> >> >>>    store i32 10, i32* %p, align 4, !tbaa !1
> >> >>>    ret void
> >> >>> }
> >> >>>
> >> >> Further to my first post, based on the definition of readnone on an
> >> >> argument, this is also incorrect.  After get_pntr() has been inlined
> >> >> into store(), we are dereferencing %p, but it is still marked
> >> >> readnone.
> >> >>
> >> >> So we seem to have a couple of issues.  First GVN seems to be making
> >> >> incorrect assumptions based on argument attributes, and secondly
> >> >> inlining can invalidate existing attributes?
> >> >
> >> > I'm not clear on the GVN issue, but looking at your example IR, the
> >> > readnone
> >> > attribute is definitely incorrect after inlining.
> >> >
> >>
> >> Yes.  I haven't investigated it any further, but the parameter was
> >> already readnone before inlining, so it looks like the attributes
> >> aren't revisited afterwards.  But that is just a guess.
> >>
> >> > A question worth asking here: does this definition of readnone make
> >> > sense?
> >> > I can see where it came from, but it seems to give very unintuitive
> >> > reasoning here.
> >> >
> >>
> >> Exactly.  That was my point in asking here before I went any further.
> >> The definition in the LangRef seems odd.  If it could still be
> >> accessed by another pointer, I can't see where the information is
> >> useful.
> >>
> >> > p.s. Is this with TOT or an earlier version?
> >>
> >> Yes, this was with TOT as of yesterday (r209294).
> >>
> >> Thanks,
> >> Rob.
> >>
> >> >
> >> > Philip
> >> >
> >> > _______________________________________________
> >> > LLVM Developers mailing list
> >> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140523/db2825ca/attachment.html>