Quentin Colombet via llvm-dev
2015-Nov-20 17:07 UTC
[llvm-dev] [AArch64] bug in shrink-wrapping
Hi Arnaud, Thanks for following up with that and sorry for the breakage. Couple of comments: MachineLoopInfo *MLI; + RegScavenger *RS; Would it make sense to use a unique_ptr here? That should eliminate the need of having explicit deletes. +; RUN: llc -mtriple=aarch64-linux-gnu -o - %s Add -enable-shrink-wrap=true and a second RUN line with -enable-shrink-wrap=false. Then add check lines for both to ensure shrink-wrapping is doing the right thing. + %0 = load i32, i32* @g1, align 4 Please use opt -instnamer to get rid of the numbered variables. Those are a pain when updating the tests :). Other than that LGTM! Cheers, -Quentin> On Nov 20, 2015, at 6:31 AM, Arnaud A. de Grandmaison <arnaud.degrandmaison at arm.com> wrote: > > +CC llvm-dev > >> -----Original Message----- >> From: Arnaud A. de Grandmaison [mailto:arnaud.degrandmaison at arm.com] >> Sent: 20 November 2015 15:28 >> To: 'qcolombet at apple.com' >> Cc: 'haicheng at codeaurora.org' >> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping >> >> Now with memory leak addressed. >> >> Cheers, >> Arnaud >> >>> -----Original Message----- >>> From: Arnaud A. de Grandmaison >> [mailto:arnaud.degrandmaison at arm.com] >>> Sent: 20 November 2015 14:42 >>> To: 'qcolombet at apple.com' >>> Cc: 'haicheng at codeaurora.org' >>> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping >>> >>> There is a memory leak in my previous patch, let me fix it. >>> >>> Cheers, >>> Arnaud >>> >>>> -----Original Message----- >>>> From: Arnaud A. de Grandmaison >>> [mailto:arnaud.degrandmaison at arm.com] >>>> Sent: 20 November 2015 12:49 >>>> To: qcolombet at apple.com >>>> Cc: 'haicheng at codeaurora.org' >>>> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping >>>> >>>> Hi Quentin, >>>> >>>> I believe the attached patch fixes the issue. Can you review it ? >>>> >>>> Cheers, >>>> Arnaud >>>> >>>>> -----Original Message----- >>>>> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf >>>>> Of via llvm-dev >>>>> Sent: 20 November 2015 05:37 >>>>> To: qcolombet at apple.com >>>>> Cc: llvm-dev at lists.llvm.org >>>>> Subject: [llvm-dev] [AArch64] bug in shrink-wrapping >>>>> >>>>> Hi Quentin, >>>>> >>>>> After shrink-wrapping was enabled as default on AArch64, llc has a >>>>> seg fault when compiling the attached .ll file on AArch64. >>>>> >>>>> My command is >>>>> >>>>> llc -mcpu=cortex-a57 bug.ll >>>>> >>>>> Best, >>>>> >>>>> Haicheng > <0001-ShrinkWrap-Teach-ShrinkWrap-to-handle-targets-requir.patch>
Arnaud A. de Grandmaison via llvm-dev
2015-Nov-20 17:55 UTC
[llvm-dev] [AArch64] bug in shrink-wrapping
> -----Original Message----- > From: qcolombet at apple.com [mailto:qcolombet at apple.com] > Sent: 20 November 2015 18:07 > To: Arnaud De Grandmaison > Cc: haicheng at codeaurora.org; llvm-dev at lists.llvm.org > Subject: Re: [llvm-dev] [AArch64] bug in shrink-wrapping > > Hi Arnaud, > > Thanks for following up with that and sorry for the breakage. > > Couple of comments: > MachineLoopInfo *MLI; > + RegScavenger *RS; > > Would it make sense to use a unique_ptr here? > That should eliminate the need of having explicit deletes.Using unique_ptr was my first attempt at fixing the memory leak :) I do not think you can, unless there is a way to "reset" (or create for the first time) the register scavenger for each function which requires it. The logical place where to put the unique_ptr would be in the runOnMachineFunction (and not as a class member) because this method has multiple exit paths, but the scavenger has to be passed around several calls then.> > +; RUN: llc -mtriple=aarch64-linux-gnu -o - %s > > Add -enable-shrink-wrap=true and a second RUN line with -enable-shrink- > wrap=false. > Then add check lines for both to ensure shrink-wrapping is doing the right > thing.I was checking here for the crash only, so not having a crash is a successfully passing test. I should maybe improve the comment. If I understand you well, you want to additionally check shrink-wrapping is doing the right thing as it seems there was a coverage hole there. I would suggest that this is added as a follow-up patch, as this is for now breaking internal bots and it may take a bit of time for me to understand what are the actual expectations from shrinkwrap.> > + %0 = load i32, i32* @g1, align 4 > Please use opt -instnamer to get rid of the numbered variables. Those area> pain when updating the tests :).Sure, will do.> > Other than that LGTM! > > Cheers, > -Quentin > > > On Nov 20, 2015, at 6:31 AM, Arnaud A. de Grandmaison > <arnaud.degrandmaison at arm.com> wrote: > > > > +CC llvm-dev > > > >> -----Original Message----- > >> From: Arnaud A. de Grandmaison > [mailto:arnaud.degrandmaison at arm.com] > >> Sent: 20 November 2015 15:28 > >> To: 'qcolombet at apple.com' > >> Cc: 'haicheng at codeaurora.org' > >> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping > >> > >> Now with memory leak addressed. > >> > >> Cheers, > >> Arnaud > >> > >>> -----Original Message----- > >>> From: Arnaud A. de Grandmaison > >> [mailto:arnaud.degrandmaison at arm.com] > >>> Sent: 20 November 2015 14:42 > >>> To: 'qcolombet at apple.com' > >>> Cc: 'haicheng at codeaurora.org' > >>> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping > >>> > >>> There is a memory leak in my previous patch, let me fix it. > >>> > >>> Cheers, > >>> Arnaud > >>> > >>>> -----Original Message----- > >>>> From: Arnaud A. de Grandmaison > >>> [mailto:arnaud.degrandmaison at arm.com] > >>>> Sent: 20 November 2015 12:49 > >>>> To: qcolombet at apple.com > >>>> Cc: 'haicheng at codeaurora.org' > >>>> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping > >>>> > >>>> Hi Quentin, > >>>> > >>>> I believe the attached patch fixes the issue. Can you review it ? > >>>> > >>>> Cheers, > >>>> Arnaud > >>>> > >>>>> -----Original Message----- > >>>>> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf > >>>>> Of via llvm-dev > >>>>> Sent: 20 November 2015 05:37 > >>>>> To: qcolombet at apple.com > >>>>> Cc: llvm-dev at lists.llvm.org > >>>>> Subject: [llvm-dev] [AArch64] bug in shrink-wrapping > >>>>> > >>>>> Hi Quentin, > >>>>> > >>>>> After shrink-wrapping was enabled as default on AArch64, llc has a > >>>>> seg fault when compiling the attached .ll file on AArch64. > >>>>> > >>>>> My command is > >>>>> > >>>>> llc -mcpu=cortex-a57 bug.ll > >>>>> > >>>>> Best, > >>>>> > >>>>> Haicheng > > <0001-ShrinkWrap-Teach-ShrinkWrap-to-handle-targets-requir.patch>
Quentin Colombet via llvm-dev
2015-Nov-20 19:57 UTC
[llvm-dev] [AArch64] bug in shrink-wrapping
> On Nov 20, 2015, at 9:55 AM, Arnaud A. de Grandmaison <arnaud.degrandmaison at arm.com> wrote: > > > >> -----Original Message----- >> From: qcolombet at apple.com [mailto:qcolombet at apple.com] >> Sent: 20 November 2015 18:07 >> To: Arnaud De Grandmaison >> Cc: haicheng at codeaurora.org; llvm-dev at lists.llvm.org >> Subject: Re: [llvm-dev] [AArch64] bug in shrink-wrapping >> >> Hi Arnaud, >> >> Thanks for following up with that and sorry for the breakage. >> >> Couple of comments: >> MachineLoopInfo *MLI; >> + RegScavenger *RS; >> >> Would it make sense to use a unique_ptr here? >> That should eliminate the need of having explicit deletes. > > Using unique_ptr was my first attempt at fixing the memory leak :) > > I do not think you can, unless there is a way to "reset" (or create for the > first time) the register scavenger for each function which requires it. > > The logical place where to put the unique_ptr would be in the > runOnMachineFunction (and not as a class member) because this method has > multiple exit paths, but the scavenger has to be passed around several calls > then.Looks like you have a better hand on this than me :).> >> >> +; RUN: llc -mtriple=aarch64-linux-gnu -o - %s >> >> Add -enable-shrink-wrap=true and a second RUN line with -enable-shrink- >> wrap=false. >> Then add check lines for both to ensure shrink-wrapping is doing the right >> thing. > > I was checking here for the crash only, so not having a crash is a > successfully passing test. I should maybe improve the comment. > > If I understand you well, you want to additionally check shrink-wrapping is > doing the right thing as it seems there was a coverage hole there. > > I would suggest that this is added as a follow-up patch, as this is for now > breaking internal bots and it may take a bit of time for me to understand > what are the actual expectations from shrink-wrap.Fair enough, please proceed! Thanks, Q.> >> >> + %0 = load i32, i32* @g1, align 4 >> Please use opt -instnamer to get rid of the numbered variables. Those are > a >> pain when updating the tests :). > > Sure, will do. > >> >> Other than that LGTM! >> >> Cheers, >> -Quentin >> >>> On Nov 20, 2015, at 6:31 AM, Arnaud A. de Grandmaison >> <arnaud.degrandmaison at arm.com> wrote: >>> >>> +CC llvm-dev >>> >>>> -----Original Message----- >>>> From: Arnaud A. de Grandmaison >> [mailto:arnaud.degrandmaison at arm.com] >>>> Sent: 20 November 2015 15:28 >>>> To: 'qcolombet at apple.com' >>>> Cc: 'haicheng at codeaurora.org' >>>> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping >>>> >>>> Now with memory leak addressed. >>>> >>>> Cheers, >>>> Arnaud >>>> >>>>> -----Original Message----- >>>>> From: Arnaud A. de Grandmaison >>>> [mailto:arnaud.degrandmaison at arm.com] >>>>> Sent: 20 November 2015 14:42 >>>>> To: 'qcolombet at apple.com' >>>>> Cc: 'haicheng at codeaurora.org' >>>>> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping >>>>> >>>>> There is a memory leak in my previous patch, let me fix it. >>>>> >>>>> Cheers, >>>>> Arnaud >>>>> >>>>>> -----Original Message----- >>>>>> From: Arnaud A. de Grandmaison >>>>> [mailto:arnaud.degrandmaison at arm.com] >>>>>> Sent: 20 November 2015 12:49 >>>>>> To: qcolombet at apple.com >>>>>> Cc: 'haicheng at codeaurora.org' >>>>>> Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping >>>>>> >>>>>> Hi Quentin, >>>>>> >>>>>> I believe the attached patch fixes the issue. Can you review it ? >>>>>> >>>>>> Cheers, >>>>>> Arnaud >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf >>>>>>> Of via llvm-dev >>>>>>> Sent: 20 November 2015 05:37 >>>>>>> To: qcolombet at apple.com >>>>>>> Cc: llvm-dev at lists.llvm.org >>>>>>> Subject: [llvm-dev] [AArch64] bug in shrink-wrapping >>>>>>> >>>>>>> Hi Quentin, >>>>>>> >>>>>>> After shrink-wrapping was enabled as default on AArch64, llc has a >>>>>>> seg fault when compiling the attached .ll file on AArch64. >>>>>>> >>>>>>> My command is >>>>>>> >>>>>>> llc -mcpu=cortex-a57 bug.ll >>>>>>> >>>>>>> Best, >>>>>>> >>>>>>> Haicheng >>> <0001-ShrinkWrap-Teach-ShrinkWrap-to-handle-targets-requir.patch>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151120/1abc35c2/attachment.html>