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