Re: [PATCH] more warning code refactoring
On 8/19/21 7:38 PM, Kewen.Lin wrote: Hi Martin, on 2021/8/20 上午12:30, Martin Sebor wrote: On 8/19/21 9:03 AM, Martin Sebor wrote: On 8/18/21 11:56 PM, Kewen.Lin wrote: Hi David, on 2021/8/19 上午11:26, David Edelsohn via Gcc-patches wrote: Hi, Martin A few PowerPC-specific testcases started failing yesterday on AIX with a strange failure mode: the compiler runs out of memory. As you may expect from telling you this in an email reply to your patch, I have bisected the failure and landed on your commit. I can alternate between the previous commit and your commit, and the failure definitely appears with your patch, although I'm unsure how your patch affected memory allocation in the compiler. Maybe moving the code changed a type of allocation or some memory no longer is being freed? To get rid of GTY variable alloc_object_size_limit looks suspicious, maybe tree objects returned by alloc_max_size after the change are out of GC's tracking? If the suspicion holds, the attached explorative diff may help. I wouldn't expect that to make a difference. There are thousands of similar calls to build_int_cst() throughout the middle end. Looking at the original patch, the change that I'm not sure about and that shouldn't have been part of the refactoring is the call to enable_ranger() in pass_waccess::execute(). It's something I was planning to do next. But even that I wouldn't expect to eat up a whole 1GB or memory. I have reproduced the excessive memory consumption with the rlwimi-0.c test and a powerpc-linux cross-compiler, and confirmed that it is indeed caused by the call to enable_ranger(). The test defines some six thousand functions so it seems that unless each call enable_ranger() is paired with some call to release the memory it allocates the memory leaks. The removal of the alloc_object_size_limit global variable doesn't have any effect on the test case. The function that used it (and now calls build_int_cst () instead) isn't called when the test is compiled (It's only called for calls to allocation functions in the source and the test case has none. Thanks for the clarification and sorry for noisy suspicion! No worries. It was a reasonable first guess. Martin BR, Kewen Let me take care of releasing the ranger memory. Martin BR, Kewen Previously, compiler bootstrap and all testcases ran with a data size of 1GB. After your change, the data size required for those particular testcases jumped to 2GB. The testcases are gcc/testsuite/gcc.target/powerpc/rlwimi-[012].c The failure is cc1: out of memory allocating 65536 bytes after a total of 1608979296 This seems like a significant memory use regression. Any ideas what happened? Not really. The patch just moved code around. I didn't make any changes that I'd expect to impact memory allocation to an appreciable extent, at least not intentionally. Let me look into it and get back to you. Martin Thanks, David
Re: [PATCH] more warning code refactoring
Hi Martin, on 2021/8/20 上午12:30, Martin Sebor wrote: > On 8/19/21 9:03 AM, Martin Sebor wrote: >> On 8/18/21 11:56 PM, Kewen.Lin wrote: >>> Hi David, >>> >>> on 2021/8/19 上午11:26, David Edelsohn via Gcc-patches wrote: Hi, Martin A few PowerPC-specific testcases started failing yesterday on AIX with a strange failure mode: the compiler runs out of memory. As you may expect from telling you this in an email reply to your patch, I have bisected the failure and landed on your commit. I can alternate between the previous commit and your commit, and the failure definitely appears with your patch, although I'm unsure how your patch affected memory allocation in the compiler. Maybe moving the code changed a type of allocation or some memory no longer is being freed? >>> >>> >>> To get rid of GTY variable alloc_object_size_limit looks suspicious, >>> maybe tree objects returned by alloc_max_size after the change are out >>> of GC's tracking? >>> >>> If the suspicion holds, the attached explorative diff may help. >> >> I wouldn't expect that to make a difference. There are thousands >> of similar calls to build_int_cst() throughout the middle end. >> >> Looking at the original patch, the change that I'm not sure about >> and that shouldn't have been part of the refactoring is the call >> to enable_ranger() in pass_waccess::execute(). It's something >> I was planning to do next. But even that I wouldn't expect to >> eat up a whole 1GB or memory. > > I have reproduced the excessive memory consumption with > the rlwimi-0.c test and a powerpc-linux cross-compiler, and > confirmed that it is indeed caused by the call to enable_ranger(). > The test defines some six thousand functions so it seems that > unless each call enable_ranger() is paired with some call to > release the memory it allocates the memory leaks. > > The removal of the alloc_object_size_limit global variable doesn't > have any effect on the test case. The function that used it (and > now calls build_int_cst () instead) isn't called when the test > is compiled (It's only called for calls to allocation functions > in the source and the test case has none. > Thanks for the clarification and sorry for noisy suspicion! BR, Kewen > Let me take care of releasing the ranger memory. > > Martin > > >> >>> >>> BR, >>> Kewen >>> Previously, compiler bootstrap and all testcases ran with a data size of 1GB. After your change, the data size required for those particular testcases jumped to 2GB. The testcases are gcc/testsuite/gcc.target/powerpc/rlwimi-[012].c The failure is cc1: out of memory allocating 65536 bytes after a total of 1608979296 This seems like a significant memory use regression. Any ideas what happened? >> >> Not really. The patch just moved code around. I didn't make any >> changes that I'd expect to impact memory allocation to an appreciable >> extent, at least not intentionally. Let me look into it and get back >> to you. >> >> Martin >> Thanks, David >> >
Re: [PATCH] more warning code refactoring
On Thu, Aug 19, 2021 at 12:53:16PM -0600, Martin Sebor wrote: > That said, I introduced > the variable in r243470 to begin with and I consider its removal > a trivially correct and appropriate part of refactoring. It is not a refactoring. It changes behaviour. Segher
Re: [PATCH] more warning code refactoring
On 8/19/21 9:36 AM, Segher Boessenkool wrote: On Thu, Aug 19, 2021 at 09:03:44AM -0600, Martin Sebor via Gcc-patches wrote: On 8/18/21 11:56 PM, Kewen.Lin wrote: To get rid of GTY variable alloc_object_size_limit looks suspicious, maybe tree objects returned by alloc_max_size after the change are out of GC's tracking? I wouldn't expect that to make a difference. There are thousands of similar calls to build_int_cst() throughout the middle end. But it does make a huge difference. It has nothing to do with the calls to build_int_cst, either. Please don't mess with GTY (and call it a refactoring, to add insult to injury) if you do not know what you are doing :-( As I explained, the leak was caused by failing to pair the call to enable_ranger() with the corresponding call to disable_ranger(). The removal of the global GTY variable has no impact on the test case, or on memory usage in general. That said, I introduced the variable in r243470 to begin with and I consider its removal a trivially correct and appropriate part of refactoring. Looking at the original patch, the change that I'm not sure about and that shouldn't have been part of the refactoring is the call to enable_ranger() in pass_waccess::execute(). It's something I was planning to do next. But even that I wouldn't expect to eat up a whole 1GB or memory. The testcase is super heavy in the instruction combiner, so you get a lot of garbage. Which is not a problem, except you made the garbage collector not pick this up, so we get a ton of garbage accumulating. Neither that nor anything else you said is relevant or helpful. On the off chance were trying to be and not just using this as an opportunity to lecture, I recommend you do your homework first next time and choose your words more carefully. Martin
Re: [PATCH] more warning code refactoring
On 8/19/21 9:03 AM, Martin Sebor wrote: On 8/18/21 11:56 PM, Kewen.Lin wrote: Hi David, on 2021/8/19 上午11:26, David Edelsohn via Gcc-patches wrote: Hi, Martin A few PowerPC-specific testcases started failing yesterday on AIX with a strange failure mode: the compiler runs out of memory. As you may expect from telling you this in an email reply to your patch, I have bisected the failure and landed on your commit. I can alternate between the previous commit and your commit, and the failure definitely appears with your patch, although I'm unsure how your patch affected memory allocation in the compiler. Maybe moving the code changed a type of allocation or some memory no longer is being freed? To get rid of GTY variable alloc_object_size_limit looks suspicious, maybe tree objects returned by alloc_max_size after the change are out of GC's tracking? If the suspicion holds, the attached explorative diff may help. I wouldn't expect that to make a difference. There are thousands of similar calls to build_int_cst() throughout the middle end. Looking at the original patch, the change that I'm not sure about and that shouldn't have been part of the refactoring is the call to enable_ranger() in pass_waccess::execute(). It's something I was planning to do next. But even that I wouldn't expect to eat up a whole 1GB or memory. I have reproduced the excessive memory consumption with the rlwimi-0.c test and a powerpc-linux cross-compiler, and confirmed that it is indeed caused by the call to enable_ranger(). The test defines some six thousand functions so it seems that unless each call enable_ranger() is paired with some call to release the memory it allocates the memory leaks. The removal of the alloc_object_size_limit global variable doesn't have any effect on the test case. The function that used it (and now calls build_int_cst () instead) isn't called when the test is compiled (It's only called for calls to allocation functions in the source and the test case has none. Let me take care of releasing the ranger memory. Martin BR, Kewen Previously, compiler bootstrap and all testcases ran with a data size of 1GB. After your change, the data size required for those particular testcases jumped to 2GB. The testcases are gcc/testsuite/gcc.target/powerpc/rlwimi-[012].c The failure is cc1: out of memory allocating 65536 bytes after a total of 1608979296 This seems like a significant memory use regression. Any ideas what happened? Not really. The patch just moved code around. I didn't make any changes that I'd expect to impact memory allocation to an appreciable extent, at least not intentionally. Let me look into it and get back to you. Martin Thanks, David
Re: [PATCH] more warning code refactoring
Hi, Kewen Good catch! The patch is in the right direction, but gimple-ssa-warn-access.cc is the first file that requires GTY and ends in ".cc". The GCC Makefile machinery to create the GTY headers performs the substitution for files with file extension ".c", so this requires more adjustment in the Makefile. Thanks, David On Thu, Aug 19, 2021 at 1:57 AM Kewen.Lin wrote: > > Hi David, > > on 2021/8/19 上午11:26, David Edelsohn via Gcc-patches wrote: > > Hi, Martin > > > > A few PowerPC-specific testcases started failing yesterday on AIX with > > a strange failure mode: the compiler runs out of memory. As you may > > expect from telling you this in an email reply to your patch, I have > > bisected the failure and landed on your commit. I can alternate > > between the previous commit and your commit, and the failure > > definitely appears with your patch, although I'm unsure how your patch > > affected memory allocation in the compiler. Maybe moving the code > > changed a type of allocation or some memory no longer is being freed? > > > > > To get rid of GTY variable alloc_object_size_limit looks suspicious, > maybe tree objects returned by alloc_max_size after the change are out > of GC's tracking? > > If the suspicion holds, the attached explorative diff may help. > > BR, > Kewen > > > Previously, compiler bootstrap and all testcases ran with a data size > > of 1GB. After your change, the data size required for those > > particular testcases jumped to 2GB. > > > > The testcases are > > > > gcc/testsuite/gcc.target/powerpc/rlwimi-[012].c > > > > The failure is > > > > cc1: out of memory allocating 65536 bytes after a total of 1608979296 > > > > This seems like a significant memory use regression. Any ideas what > > happened? > > > > Thanks, David > >
Re: [PATCH] more warning code refactoring
On Thu, Aug 19, 2021 at 09:03:44AM -0600, Martin Sebor via Gcc-patches wrote: > On 8/18/21 11:56 PM, Kewen.Lin wrote: > >To get rid of GTY variable alloc_object_size_limit looks suspicious, > >maybe tree objects returned by alloc_max_size after the change are out > >of GC's tracking? > > I wouldn't expect that to make a difference. There are thousands > of similar calls to build_int_cst() throughout the middle end. But it does make a huge difference. It has nothing to do with the calls to build_int_cst, either. Please don't mess with GTY (and call it a refactoring, to add insult to injury) if you do not know what you are doing :-( > Looking at the original patch, the change that I'm not sure about > and that shouldn't have been part of the refactoring is the call > to enable_ranger() in pass_waccess::execute(). It's something > I was planning to do next. But even that I wouldn't expect to > eat up a whole 1GB or memory. The testcase is super heavy in the instruction combiner, so you get a lot of garbage. Which is not a problem, except you made the garbage collector not pick this up, so we get a ton of garbage accumulating. Segher
Re: [PATCH] more warning code refactoring
On 8/18/21 11:56 PM, Kewen.Lin wrote: Hi David, on 2021/8/19 上午11:26, David Edelsohn via Gcc-patches wrote: Hi, Martin A few PowerPC-specific testcases started failing yesterday on AIX with a strange failure mode: the compiler runs out of memory. As you may expect from telling you this in an email reply to your patch, I have bisected the failure and landed on your commit. I can alternate between the previous commit and your commit, and the failure definitely appears with your patch, although I'm unsure how your patch affected memory allocation in the compiler. Maybe moving the code changed a type of allocation or some memory no longer is being freed? To get rid of GTY variable alloc_object_size_limit looks suspicious, maybe tree objects returned by alloc_max_size after the change are out of GC's tracking? If the suspicion holds, the attached explorative diff may help. I wouldn't expect that to make a difference. There are thousands of similar calls to build_int_cst() throughout the middle end. Looking at the original patch, the change that I'm not sure about and that shouldn't have been part of the refactoring is the call to enable_ranger() in pass_waccess::execute(). It's something I was planning to do next. But even that I wouldn't expect to eat up a whole 1GB or memory. BR, Kewen Previously, compiler bootstrap and all testcases ran with a data size of 1GB. After your change, the data size required for those particular testcases jumped to 2GB. The testcases are gcc/testsuite/gcc.target/powerpc/rlwimi-[012].c The failure is cc1: out of memory allocating 65536 bytes after a total of 1608979296 This seems like a significant memory use regression. Any ideas what happened? Not really. The patch just moved code around. I didn't make any changes that I'd expect to impact memory allocation to an appreciable extent, at least not intentionally. Let me look into it and get back to you. Martin Thanks, David
Re: [PATCH] more warning code refactoring
Hi! On Thu, Aug 19, 2021 at 01:56:56PM +0800, Kewen.Lin via Gcc-patches wrote: > on 2021/8/19 上午11:26, David Edelsohn via Gcc-patches wrote: > > A few PowerPC-specific testcases started failing yesterday on AIX with > > a strange failure mode: the compiler runs out of memory. As you may > > expect from telling you this in an email reply to your patch, I have > > bisected the failure and landed on your commit. I can alternate > > between the previous commit and your commit, and the failure > > definitely appears with your patch, although I'm unsure how your patch > > affected memory allocation in the compiler. Maybe moving the code > > changed a type of allocation or some memory no longer is being freed? > > To get rid of GTY variable alloc_object_size_limit looks suspicious, > maybe tree objects returned by alloc_max_size after the change are out > of GC's tracking? That looks exactly right. Thanks Ke Wen! This also means this was not a refactoring at all -- nothing changing anything to do with GC is ever a refactoring. Refactorings are trivial one-to-one code transforms that do not change semantics at all. Segher
Re: [PATCH] more warning code refactoring
Hi David,
on 2021/8/19 上午11:26, David Edelsohn via Gcc-patches wrote:
> Hi, Martin
>
> A few PowerPC-specific testcases started failing yesterday on AIX with
> a strange failure mode: the compiler runs out of memory. As you may
> expect from telling you this in an email reply to your patch, I have
> bisected the failure and landed on your commit. I can alternate
> between the previous commit and your commit, and the failure
> definitely appears with your patch, although I'm unsure how your patch
> affected memory allocation in the compiler. Maybe moving the code
> changed a type of allocation or some memory no longer is being freed?
>
To get rid of GTY variable alloc_object_size_limit looks suspicious,
maybe tree objects returned by alloc_max_size after the change are out
of GC's tracking?
If the suspicion holds, the attached explorative diff may help.
BR,
Kewen
> Previously, compiler bootstrap and all testcases ran with a data size
> of 1GB. After your change, the data size required for those
> particular testcases jumped to 2GB.
>
> The testcases are
>
> gcc/testsuite/gcc.target/powerpc/rlwimi-[012].c
>
> The failure is
>
> cc1: out of memory allocating 65536 bytes after a total of 1608979296
>
> This seems like a significant memory use regression. Any ideas what happened?
>
> Thanks, David
>
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 6653e9e2142..9aefed47be8 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -2717,7 +2717,7 @@ GTFILES = $(CPPLIB_H) $(srcdir)/input.h
$(srcdir)/coretypes.h \
$(srcdir)/sancov.c \
$(srcdir)/ipa-devirt.c \
$(srcdir)/internal-fn.h \
- $(srcdir)/calls.c \
+ $(srcdir)/gimple-ssa-warn-access.cc \
$(srcdir)/omp-general.h \
@all_gtfiles@
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index f3efe564af0..267ef987edd 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -2301,6 +2301,9 @@ pass_waccess::gate (function *)
|| warn_mismatched_new_delete);
}
+/* The limit set by -Walloc-larger-than=. */
+static GTY(()) tree alloc_object_size_limit;
+
/* Initialize ALLOC_OBJECT_SIZE_LIMIT based on the -Walloc-size-larger-than=
setting if the option is specified, or to the maximum object size if it
is not. Return the initialized value. */
@@ -2308,11 +2311,16 @@ pass_waccess::gate (function *)
static tree
alloc_max_size (void)
{
+ if (alloc_object_size_limit)
+return alloc_object_size_limit;
+
HOST_WIDE_INT limit = warn_alloc_size_limit;
if (limit == HOST_WIDE_INT_MAX)
limit = tree_to_shwi (TYPE_MAX_VALUE (ptrdiff_type_node));
- return build_int_cst (size_type_node, limit);
+ alloc_object_size_limit = build_int_cst (size_type_node, limit);
+
+ return alloc_object_size_limit;
}
/* Diagnose a call EXP to function FN decorated with attribute alloc_size
@@ -3328,3 +3336,6 @@ make_pass_warn_access (gcc::context *ctxt)
{
return new pass_waccess (ctxt);
}
+
+/* Tell the garbage collector about GTY markers in this source file. */
+#include "gt-gimple-ssa-warn-access.h"
Re: [PATCH] more warning code refactoring
Hi, Martin A few PowerPC-specific testcases started failing yesterday on AIX with a strange failure mode: the compiler runs out of memory. As you may expect from telling you this in an email reply to your patch, I have bisected the failure and landed on your commit. I can alternate between the previous commit and your commit, and the failure definitely appears with your patch, although I'm unsure how your patch affected memory allocation in the compiler. Maybe moving the code changed a type of allocation or some memory no longer is being freed? Previously, compiler bootstrap and all testcases ran with a data size of 1GB. After your change, the data size required for those particular testcases jumped to 2GB. The testcases are gcc/testsuite/gcc.target/powerpc/rlwimi-[012].c The failure is cc1: out of memory allocating 65536 bytes after a total of 1608979296 This seems like a significant memory use regression. Any ideas what happened? Thanks, David
Re: [PATCH] more warning code refactoring
On 8/17/21 2:51 AM, Richard Biener wrote: On Tue, Aug 17, 2021 at 3:52 AM Martin Sebor via Gcc-patches wrote: The attached patch continues with the move of warning code from builtins.c and calls.c into a more suitable home. As before, it is mostly free of functional changes. The one exception is that as pleasant a side-effect, moving the attribute access checking from initialize_argument_information() in calls.c to the new warning pass also happens to fix PR 101854. This is thanks to the latter iterating over function arguments explicitly provided in the program and not having to worry about skipping over the additional pointer argument synthesized for calls to functions that return a large struct by value that the former function sneaks into the argument list. Tested on x86_64-linux. OK. Jit testing exposed a bug due to an uninitialized variable (oddly tests for no other front end did so). I fixed it and (after retesting the result) committed r12-2976. Thanks Martin
Re: [PATCH] more warning code refactoring
On Tue, Aug 17, 2021 at 3:52 AM Martin Sebor via Gcc-patches wrote: > > The attached patch continues with the move of warning code from > builtins.c and calls.c into a more suitable home. As before, it > is mostly free of functional changes. The one exception is that > as pleasant a side-effect, moving the attribute access checking > from initialize_argument_information() in calls.c to the new > warning pass also happens to fix PR 101854. This is thanks to > the latter iterating over function arguments explicitly provided > in the program and not having to worry about skipping over > the additional pointer argument synthesized for calls to functions > that return a large struct by value that the former function sneaks > into the argument list. > > Tested on x86_64-linux. OK. Thanks, Richard. > Martin > > Previous patches in this series: > https://gcc.gnu.org/pipermail/gcc-patches/2021-August/576821.html > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575377.html
