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

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.

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.

p.s. Is this with TOT or an earlier version?

Philip

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

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>