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

[Quentin Colombet via llvm-dev]

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