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
bug.ll (8.77 KB)
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
From: qcolombet@apple.com [mailto:qcolombet@apple.com]
Sent: 20 November 2015 18:07
To: Arnaud De Grandmaison
Cc: haicheng@codeaurora.org; llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] [AArch64] bug in shrink-wrappingHi 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.
From: qcolombet@apple.com [mailto:qcolombet@apple.com]
Sent: 20 November 2015 18:07
To: Arnaud De Grandmaison
Cc: haicheng@codeaurora.org; llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] [AArch64] bug in shrink-wrappingHi 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.
From: qcolombet@apple.com [mailto:qcolombet@apple.com]
Sent: 20 November 2015 18:07
To: Arnaud De Grandmaison
Cc: haicheng@codeaurora.org; llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] [AArch64] bug in shrink-wrappingHi 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 understandwhat are the actual expectations from shrink-wrap.
Fair enough, please proceed!
FWIW I’m running into this as well
-eric
FYI, I gave a look today at the generated code with -enable-shrink-wrap=true and -enable-shrink-wrap=false … and they are the same.
Cheers,
Arnaud