| Message ID | a54d924d-7452-aebb-d765-e8519ba9c4bc@suse.cz |
|---|---|
| State | New |
| Headers | show |
On 11/23/2016 09:13 AM, Martin Liška wrote: > Hello. > > As described in the PR, the patch fixes profiled bootstrap on x86_64-linux-gnu. > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. And > profiled bootstrap on x86_64-linux-gnu finishes successfully. > > Ready to be installed? > Martin > > > 0001-Fix-PR-bootstrap-78493.patch > > > From 8b7cd9a83cd14f7a15f39e105ccd78e143ec84f2 Mon Sep 17 00:00:00 2001 > From: marxin <mliska@suse.cz> > Date: Wed, 23 Nov 2016 14:08:52 +0100 > Subject: [PATCH] Fix PR bootstrap/78493 > > gcc/ChangeLog: > > 2016-11-23 Martin Liska <mliska@suse.cz> > > PR bootstrap/78493 > * vec.h (~auto_vec): Do va_heap::release just if > this->m_vec == &m_auto. That would help compiler not to > trigger -Werror=free-nonheap-object. It's probably the case that the profiling information inhibited a jump thread optimization (path was considered cold and thus duplication/isolation not profitable) which left the dead path in the IL. I don't like that we're essentially inlining the ::release method. At the least we should have a pair of comments. In the destructor we should indicate why we've inlined the release method. In the release method we should make a note that if the release method is changed that a suitable change to the auto_vec destructor may be needed. Alternately, given all the problems we have with this kind of problem, we should seriously consider throttling back what we consider an error during a profiled bootstrap. This kind of stuff is a maintenance nightmare with minimal value. jeff
On Wed, Nov 23, 2016 at 7:33 PM, Jeff Law <law@redhat.com> wrote: > On 11/23/2016 09:13 AM, Martin Liška wrote: >> >> Hello. >> >> As described in the PR, the patch fixes profiled bootstrap on >> x86_64-linux-gnu. >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> And >> profiled bootstrap on x86_64-linux-gnu finishes successfully. >> >> Ready to be installed? >> Martin >> >> >> 0001-Fix-PR-bootstrap-78493.patch >> >> >> From 8b7cd9a83cd14f7a15f39e105ccd78e143ec84f2 Mon Sep 17 00:00:00 2001 >> From: marxin <mliska@suse.cz> >> Date: Wed, 23 Nov 2016 14:08:52 +0100 >> Subject: [PATCH] Fix PR bootstrap/78493 >> >> gcc/ChangeLog: >> >> 2016-11-23 Martin Liska <mliska@suse.cz> >> >> PR bootstrap/78493 >> * vec.h (~auto_vec): Do va_heap::release just if >> this->m_vec == &m_auto. That would help compiler not to >> trigger -Werror=free-nonheap-object. > > It's probably the case that the profiling information inhibited a jump > thread optimization (path was considered cold and thus duplication/isolation > not profitable) which left the dead path in the IL. > > I don't like that we're essentially inlining the ::release method. At the > least we should have a pair of comments. In the destructor we should > indicate why we've inlined the release method. In the release method we > should make a note that if the release method is changed that a suitable > change to the auto_vec destructor may be needed. I don't like the patch either. > Alternately, given all the problems we have with this kind of problem, we > should seriously consider throttling back what we consider an error during a > profiled bootstrap. This kind of stuff is a maintenance nightmare with > minimal value. I think we've always communicated that all non-standard bootstrap configs need -Wdisable-werror. Richard. > jeff >
On Thu, Nov 24, 2016 at 09:26:25AM +0100, Richard Biener wrote: > > Alternately, given all the problems we have with this kind of problem, we > > should seriously consider throttling back what we consider an error during a > > profiled bootstrap. This kind of stuff is a maintenance nightmare with > > minimal value. > > I think we've always communicated that all non-standard bootstrap configs > need -Wdisable-werror. Normal profiledbootstrap (when not using -O3 or similar) has been always -Werror clean and it would be nice if it stays so. Jakub
On Thu, Nov 24, 2016 at 9:36 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Nov 24, 2016 at 09:26:25AM +0100, Richard Biener wrote: >> > Alternately, given all the problems we have with this kind of problem, we >> > should seriously consider throttling back what we consider an error during a >> > profiled bootstrap. This kind of stuff is a maintenance nightmare with >> > minimal value. >> >> I think we've always communicated that all non-standard bootstrap configs >> need -Wdisable-werror. > > Normal profiledbootstrap (when not using -O3 or similar) has been always > -Werror clean and it would be nice if it stays so. Oh, this was just profiledbootstrap. Agreed - though the proposed patch is still too ugly for my taste. Richard. > > Jakub
On Thu, Nov 24, 2016 at 09:51:49AM +0100, Richard Biener wrote: > On Thu, Nov 24, 2016 at 9:36 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Nov 24, 2016 at 09:26:25AM +0100, Richard Biener wrote: > >> > Alternately, given all the problems we have with this kind of problem, we > >> > should seriously consider throttling back what we consider an error during a > >> > profiled bootstrap. This kind of stuff is a maintenance nightmare with > >> > minimal value. > >> > >> I think we've always communicated that all non-standard bootstrap configs > >> need -Wdisable-werror. > > > > Normal profiledbootstrap (when not using -O3 or similar) has been always > > -Werror clean and it would be nice if it stays so. > > Oh, this was just profiledbootstrap. Agreed - though the proposed patch is > still too ugly for my taste. Agreed on that. Jakub
From 8b7cd9a83cd14f7a15f39e105ccd78e143ec84f2 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 23 Nov 2016 14:08:52 +0100 Subject: [PATCH] Fix PR bootstrap/78493 gcc/ChangeLog: 2016-11-23 Martin Liska <mliska@suse.cz> PR bootstrap/78493 * vec.h (~auto_vec): Do va_heap::release just if this->m_vec == &m_auto. That would help compiler not to trigger -Werror=free-nonheap-object. --- gcc/vec.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/gcc/vec.h b/gcc/vec.h index 14fb2a6..d2d253b 100644 --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1272,7 +1272,14 @@ public: ~auto_vec () { - this->release (); + if (this->m_vec == &m_auto) + { + gcc_checking_assert (this->using_auto_storage ()); + this->m_vec->m_vecpfx.m_num = 0; + return; + } + + va_heap::release (this->m_vec); } private: -- 2.10.2