[llvm-dev] [AArch64] bug in shrink-wrapping

Arnaud A. de Grandmaison via llvm-dev llvm-dev at lists.llvm.org
Fri Nov 20 09:55:58 PST 2015



> -----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 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>





More information about the llvm-dev mailing list