llvm dev - May 2014 - [LLVMdev] GVN incorrectly handling readnone parameter attribute?

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/20140522/3ea40ef0/attachment.html>

Hi Nick,

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

Strangely enough, my first conclusion was that %p was being marked
readnone incorrectly as it wasn't handling the copy via @get_addr.
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?

Thanks,
Rob.


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
>
>

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>