[PATCH] InlineFunction doesn't update InlineFunctionInfo with allocas created for byval arguments

[Manman Ren]

Hi Julien,

The updated patch looks good overall.
Since there is refactoring going on, can you separate to two patches, one
with refactoring but no functionality change, to make review easier?

Thanks,
Manman



On Thu, Apr 10, 2014 at 2:22 PM, Julien Lerouge <jlerouge at apple.com> wrote:

> On Tue, Apr 08, 2014 at 09:35:40PM -0700, Julien Lerouge wrote:
> > On Tue, Apr 08, 2014 at 05:40:58PM -0700, Manman Ren wrote:
> > > On Tue, Apr 8, 2014 at 3:18 PM, Julien Lerouge <jlerouge at apple.com>
> wrote:
> > >
> > > > On Tue, Apr 08, 2014 at 02:26:52PM -0700, Julien Lerouge wrote:
> > > > > On Tue, Apr 08, 2014 at 10:30:44AM -0700, Manman Ren wrote:
> > > > > > On Fri, Apr 4, 2014 at 5:15 PM, Julien Lerouge <
> julien.lerouge at m4x.org
> > > > >wrote:
> > > > > >
> > > > > > > When llvm::InlineFunction inlines a function that has byval
> > > > arguments,
> > > > > > > it creates allocas in the caller. Those allocas aren't
> inserted in
> > > > the
> > > > > > > InlineFunctionInfo data structure. So after inlining, if the
> client
> > > > code
> > > > > > > wants to know where are the allocas that were created, it will
> miss
> > > > > > > those.
> > > > > > >
> > > > > > > Should InlineFunctionInfo contain these allocas, or is the
> omission
> > > > > > > deliberate ?
> > > > > > >
> > > > > >
> > > > > > Hi Julien,
> > > > > >
> > > > > > I don't think the omission is deliberate, but I may be wrong.
> > > > > > Do you have a testing case?
> > > > > >
> > > > > > Thanks,
> > > > > > Manman
> > > > > >
> > > > >
> > > > > Yeah, sorry, I'll add one and resend the patch.
> > > > >
> > > > > Thanks,
> > > > > Julien
> > > > >
> > > >
> > > > Attached below. The test just inlines a function with byval and makes
> > > > sure the lifetime marker gets created. Without the fix, no lifetime
> > > > marker is created.
> > > >
> > >
> > > Hi Julien,
> > >
> > > Thanks for the testing case. However I don't think it is the right fix.
> > > llvm.lifetime intrinsics are optimization hints. With this patch, we
> get
> > > define i32 @main(i32 %argc, i8** %argv) {
> > > entry:
> > >   %gFoo = alloca %struct.foo, align 8
> > >   %tmp = bitcast %struct.foo* %gFoo to i8*
> > >   %tmp1 = bitcast %struct.foo* @gFoo to i8*
> > >   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %tmp, i8* %tmp1, i64
> ptrtoint
> > > (%struct.foo* getelementptr (%struct.foo* null, i32 1) to i64), i32 1,
> i1
> > > false)
> > >   %0 = bitcast %struct.foo* %gFoo to i8*
> > >   call void @llvm.lifetime.start(i64 -1, i8* %0)
> > >   %a1.i = getelementptr inbounds %struct.foo* %gFoo, i32 0, i32 1
> > >   %arrayidx.i = getelementptr inbounds [16 x i32]* %a1.i, i32 0, i32
> %argc
> > >   %tmp2.i = load i32* %arrayidx.i, align 1
> > >   %1 = bitcast %struct.foo* %gFoo to i8*
> > >   call void @llvm.lifetime.end(i64 -1, i8* %1)
> > >   ret i32 %tmp2.i
> > > }
> > >
> > > lifetime.start after memcpy seems to be wrong because %gFoo is already
> live
> > > at memcpy.
> > > Do you see a performance issue or you happen to notice this when
> browsing
> > > the code?
> >
> > Ah, good catch. Thanks. I don't have any performance issue no. I just
> > have some code that uses the InlineInfo and noticed the byval allocas
> > weren't in there.
> >
> > When I looked into writing a test case, the lifetime markers looked like
> > one of the only real users of the InlineInfo, so that's why the test
> > case is checking that. Unfortunately, I didn't verify the markers were
> > at the right place.
> >
> > I'll check if it's an easy fix, and if not, I'll just change my code to
> > not assume the byval allocas are accounted for.
> >
>
> Alright, attached a new version. It involves splitting the byval
> handling so that we inject the memcopy after the code has been cloned,
> in the first new block. By doing so, the lifetime markers insertion
> doesn't need to change.
>
> Hope this helps,
> Julien
>
> --
> Julien Lerouge
> PGP Key Id: 0xB1964A62
> PGP Fingerprint: 392D 4BAD DB8B CE7F 4E5F FA3C 62DB 4AA7 B196 4A62
> PGP Public Key from: keyserver.pgp.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140411/d043d76d/attachment.html>