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

[Julien Lerouge]

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.

Thanks,

-- 
Julien Lerouge