| Message ID | 029bb8e7-1689-e597-f9f9-ae929c8f54f0@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix profile_quality sanity check. | expand |
On 01/18/2018 03:59 PM, Martin Liška wrote: > Hi. > > Following patch adds a new enum value so that we don't see following warning: > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01211.html > Hi, with the patch, I still see the same warning. And not surprisingly, given that profile_precise is still 7 and m_quality is still a 3 bits wide bitfield. So, I don't quite understand why you think that the patch would address the warning. Thanks, - Tom > Apart from that I decided to not to number values of the enum as it uses > default number. Is it welcome? > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > > Ready to be installed? > Martin > > gcc/ChangeLog: > > 2018-01-18 Martin Liska <mliska@suse.cz> > > * profile-count.h (enum profile_quality): Add > profile_uninitialized as the first value. Do not number values > as they are zero based. > --- > gcc/profile-count.h | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > 0001-Fix-profile_quality-sanity-check.patch > > > diff --git a/gcc/profile-count.h b/gcc/profile-count.h > index 7a43917ebbc..e899963118b 100644 > --- a/gcc/profile-count.h > +++ b/gcc/profile-count.h > @@ -26,34 +26,36 @@ struct function; > /* Quality of the profile count. Because gengtype does not support enums > inside of classes, this is in global namespace. */ > enum profile_quality { > + /* Uninitialized value. */ > + profile_uninitialized, > /* Profile is based on static branch prediction heuristics and may > or may not match reality. It is local to function and can not be compared > inter-procedurally. Never used by probabilities (they are always local). > */ > - profile_guessed_local = 1, > + profile_guessed_local, > /* Profile was read by feedback and was 0, we used local heuristics to guess > better. This is the case of functions not run in profile fedback. > Never used by probabilities. */ > - profile_guessed_global0 = 2, > + profile_guessed_global0, > > /* Same as profile_guessed_global0 but global count is adjusted 0. */ > - profile_guessed_global0adjusted = 3, > + profile_guessed_global0adjusted, > > /* Profile is based on static branch prediction heuristics. It may or may > not reflect the reality but it can be compared interprocedurally > (for example, we inlined function w/o profile feedback into function > with feedback and propagated from that). > Never used by probablities. */ > - profile_guessed = 4, > + profile_guessed, > /* Profile was determined by autofdo. */ > - profile_afdo = 5, > + profile_afdo, > /* Profile was originally based on feedback but it was adjusted > by code duplicating optimization. It may not precisely reflect the > particular code path. */ > - profile_adjusted = 6, > + profile_adjusted, > /* Profile was read from profile feedback or determined by accurate static > method. */ > - profile_precise = 7 > + profile_precise > }; > > /* The base value for branch probability notes and edge probabilities. */ >
On 01/18/2018 04:57 PM, Tom de Vries wrote: > On 01/18/2018 03:59 PM, Martin Liška wrote: >> Hi. >> >> Following patch adds a new enum value so that we don't see following warning: >> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01211.html >> > > Hi, > > with the patch, I still see the same warning. > > And not surprisingly, given that profile_precise is still 7 and m_quality is still a 3 bits wide bitfield. Hi. Sorry I was too eager and I haven't realized that 2^4 - 1 can't fit 8 values ;) Thus I'm suggesting to simply removal of the sanity checking as it does not make sense to enlarge the bit enum. And I'm also suggesting to not to number values of the enum. Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Ready to be installed? Martin > > So, I don't quite understand why you think that the patch would address the warning. > > Thanks, > - Tom > >> Apart from that I decided to not to number values of the enum as it uses >> default number. Is it welcome? >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> >> Ready to be installed? >> Martin >> >> gcc/ChangeLog: >> >> 2018-01-18 Martin Liska <mliska@suse.cz> >> >> * profile-count.h (enum profile_quality): Add >> profile_uninitialized as the first value. Do not number values >> as they are zero based. >> --- >> gcc/profile-count.h | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> >> >> 0001-Fix-profile_quality-sanity-check.patch >> >> >> diff --git a/gcc/profile-count.h b/gcc/profile-count.h >> index 7a43917ebbc..e899963118b 100644 >> --- a/gcc/profile-count.h >> +++ b/gcc/profile-count.h >> @@ -26,34 +26,36 @@ struct function; >> /* Quality of the profile count. Because gengtype does not support enums >> inside of classes, this is in global namespace. */ >> enum profile_quality { >> + /* Uninitialized value. */ >> + profile_uninitialized, >> /* Profile is based on static branch prediction heuristics and may >> or may not match reality. It is local to function and can not be compared >> inter-procedurally. Never used by probabilities (they are always local). >> */ >> - profile_guessed_local = 1, >> + profile_guessed_local, >> /* Profile was read by feedback and was 0, we used local heuristics to guess >> better. This is the case of functions not run in profile fedback. >> Never used by probabilities. */ >> - profile_guessed_global0 = 2, >> + profile_guessed_global0, >> /* Same as profile_guessed_global0 but global count is adjusted 0. */ >> - profile_guessed_global0adjusted = 3, >> + profile_guessed_global0adjusted, >> /* Profile is based on static branch prediction heuristics. It may or may >> not reflect the reality but it can be compared interprocedurally >> (for example, we inlined function w/o profile feedback into function >> with feedback and propagated from that). >> Never used by probablities. */ >> - profile_guessed = 4, >> + profile_guessed, >> /* Profile was determined by autofdo. */ >> - profile_afdo = 5, >> + profile_afdo, >> /* Profile was originally based on feedback but it was adjusted >> by code duplicating optimization. It may not precisely reflect the >> particular code path. */ >> - profile_adjusted = 6, >> + profile_adjusted, >> /* Profile was read from profile feedback or determined by accurate static >> method. */ >> - profile_precise = 7 >> + profile_precise >> }; >> /* The base value for branch probability notes and edge probabilities. */ >> > From 0656d0dce5c26cf206ad4fcb21809a4aeb02ec42 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Thu, 18 Jan 2018 13:26:27 +0100 Subject: [PATCH] Remove profile_quality sanity check. gcc/ChangeLog: 2018-01-18 Martin Liska <mliska@suse.cz> * profile-count.h (enum profile_quality): Do not number values as they are zero based. (profile_count::verify): Remove sanity check. (profile_probability::verify): Remove sanity check. --- gcc/profile-count.h | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/gcc/profile-count.h b/gcc/profile-count.h index 7a43917ebbc..74ec9b465d3 100644 --- a/gcc/profile-count.h +++ b/gcc/profile-count.h @@ -30,30 +30,30 @@ enum profile_quality { or may not match reality. It is local to function and can not be compared inter-procedurally. Never used by probabilities (they are always local). */ - profile_guessed_local = 1, + profile_guessed_local, /* Profile was read by feedback and was 0, we used local heuristics to guess better. This is the case of functions not run in profile fedback. Never used by probabilities. */ - profile_guessed_global0 = 2, + profile_guessed_global0, /* Same as profile_guessed_global0 but global count is adjusted 0. */ - profile_guessed_global0adjusted = 3, + profile_guessed_global0adjusted, /* Profile is based on static branch prediction heuristics. It may or may not reflect the reality but it can be compared interprocedurally (for example, we inlined function w/o profile feedback into function with feedback and propagated from that). Never used by probablities. */ - profile_guessed = 4, + profile_guessed, /* Profile was determined by autofdo. */ - profile_afdo = 5, + profile_afdo, /* Profile was originally based on feedback but it was adjusted by code duplicating optimization. It may not precisely reflect the particular code path. */ - profile_adjusted = 6, + profile_adjusted, /* Profile was read from profile feedback or determined by accurate static method. */ - profile_precise = 7 + profile_precise }; /* The base value for branch probability notes and edge probabilities. */ @@ -505,8 +505,6 @@ public: /* Return false if profile_probability is bogus. */ bool verify () const { - gcc_checking_assert (profile_guessed_local <= m_quality - && m_quality <= profile_precise); if (m_val == uninitialized_probability) return m_quality == profile_guessed; else if (m_quality < profile_guessed) @@ -786,8 +784,6 @@ public: /* Return false if profile_count is bogus. */ bool verify () const { - gcc_checking_assert (profile_guessed_local <= m_quality - && m_quality <= profile_precise); return m_val != uninitialized_count || m_quality == profile_guessed_local; }
On 01/19/2018 01:11 PM, Martin Liška wrote: > On 01/18/2018 04:57 PM, Tom de Vries wrote: >> On 01/18/2018 03:59 PM, Martin Liška wrote: >>> Hi. >>> >>> Following patch adds a new enum value so that we don't see following warning: >>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01211.html >>> >> >> Hi, >> >> with the patch, I still see the same warning. >> >> And not surprisingly, given that profile_precise is still 7 and m_quality is still a 3 bits wide bitfield. > > Hi. > > Sorry I was too eager and I haven't realized that 2^4 - 1 can't fit 8 values ;) > Thus I'm suggesting to simply removal of the sanity checking as it does not make > sense to enlarge the bit enum. And I'm also suggesting to not to number values of the > enum. How about keeping profile_uninitialized at the zero value location and asserting m_quality != profile_uninitialized ? Thanks, - Tom > 0001-Remove-profile_quality-sanity-check.patch > > > From 0656d0dce5c26cf206ad4fcb21809a4aeb02ec42 Mon Sep 17 00:00:00 2001 > From: marxin <mliska@suse.cz> > Date: Thu, 18 Jan 2018 13:26:27 +0100 > Subject: [PATCH] Remove profile_quality sanity check. > > gcc/ChangeLog: > > 2018-01-18 Martin Liska <mliska@suse.cz> > > * profile-count.h (enum profile_quality): Do not number values > as they are zero based. > (profile_count::verify): Remove sanity check. > (profile_probability::verify): Remove sanity check. > --- > gcc/profile-count.h | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/gcc/profile-count.h b/gcc/profile-count.h > index 7a43917ebbc..74ec9b465d3 100644 > --- a/gcc/profile-count.h > +++ b/gcc/profile-count.h > @@ -30,30 +30,30 @@ enum profile_quality { > or may not match reality. It is local to function and can not be compared > inter-procedurally. Never used by probabilities (they are always local). > */ > - profile_guessed_local = 1, > + profile_guessed_local, > /* Profile was read by feedback and was 0, we used local heuristics to guess > better. This is the case of functions not run in profile fedback. > Never used by probabilities. */ > - profile_guessed_global0 = 2, > + profile_guessed_global0, > > /* Same as profile_guessed_global0 but global count is adjusted 0. */ > - profile_guessed_global0adjusted = 3, > + profile_guessed_global0adjusted, > > /* Profile is based on static branch prediction heuristics. It may or may > not reflect the reality but it can be compared interprocedurally > (for example, we inlined function w/o profile feedback into function > with feedback and propagated from that). > Never used by probablities. */ > - profile_guessed = 4, > + profile_guessed, > /* Profile was determined by autofdo. */ > - profile_afdo = 5, > + profile_afdo, > /* Profile was originally based on feedback but it was adjusted > by code duplicating optimization. It may not precisely reflect the > particular code path. */ > - profile_adjusted = 6, > + profile_adjusted, > /* Profile was read from profile feedback or determined by accurate static > method. */ > - profile_precise = 7 > + profile_precise > }; > > /* The base value for branch probability notes and edge probabilities. */ > @@ -505,8 +505,6 @@ public: > /* Return false if profile_probability is bogus. */ > bool verify () const > { > - gcc_checking_assert (profile_guessed_local <= m_quality > - && m_quality <= profile_precise); > if (m_val == uninitialized_probability) > return m_quality == profile_guessed; > else if (m_quality < profile_guessed) > @@ -786,8 +784,6 @@ public: > /* Return false if profile_count is bogus. */ > bool verify () const > { > - gcc_checking_assert (profile_guessed_local <= m_quality > - && m_quality <= profile_precise); > return m_val != uninitialized_count || m_quality == profile_guessed_local; > } > >
On 01/19/2018 02:21 PM, Tom de Vries wrote: > How about keeping profile_uninitialized at the zero value location and asserting m_quality != profile_uninitialized ? > > Thanks, > - Tom Yes, that would be possible. Can you please test that the patch does not generate warnings? I'm running regression tests. Martin From e1159c2404947f675200efc4476e7e0994b81101 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Fri, 19 Jan 2018 15:27:40 +0100 Subject: [PATCH] Fix profile_quality sanity check. gcc/ChangeLog: 2018-01-18 Martin Liska <mliska@suse.cz> * profile-count.h (enum profile_quality): Add profile_uninitialized as the first value. Do not number values as they are zero based. (profile_count::verify): Update sanity check. (profile_probability::verify): Likewise. --- gcc/profile-count.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/gcc/profile-count.h b/gcc/profile-count.h index 7a43917ebbc..828d6d0ee4b 100644 --- a/gcc/profile-count.h +++ b/gcc/profile-count.h @@ -26,34 +26,36 @@ struct function; /* Quality of the profile count. Because gengtype does not support enums inside of classes, this is in global namespace. */ enum profile_quality { + /* Uninitialized value. */ + profile_uninitialized, /* Profile is based on static branch prediction heuristics and may or may not match reality. It is local to function and can not be compared inter-procedurally. Never used by probabilities (they are always local). */ - profile_guessed_local = 1, + profile_guessed_local, /* Profile was read by feedback and was 0, we used local heuristics to guess better. This is the case of functions not run in profile fedback. Never used by probabilities. */ - profile_guessed_global0 = 2, + profile_guessed_global0, /* Same as profile_guessed_global0 but global count is adjusted 0. */ - profile_guessed_global0adjusted = 3, + profile_guessed_global0adjusted, /* Profile is based on static branch prediction heuristics. It may or may not reflect the reality but it can be compared interprocedurally (for example, we inlined function w/o profile feedback into function with feedback and propagated from that). Never used by probablities. */ - profile_guessed = 4, + profile_guessed, /* Profile was determined by autofdo. */ - profile_afdo = 5, + profile_afdo, /* Profile was originally based on feedback but it was adjusted by code duplicating optimization. It may not precisely reflect the particular code path. */ - profile_adjusted = 6, + profile_adjusted, /* Profile was read from profile feedback or determined by accurate static method. */ - profile_precise = 7 + profile_precise }; /* The base value for branch probability notes and edge probabilities. */ @@ -505,8 +507,7 @@ public: /* Return false if profile_probability is bogus. */ bool verify () const { - gcc_checking_assert (profile_guessed_local <= m_quality - && m_quality <= profile_precise); + gcc_checking_assert (m_quality != profile_uninitialized); if (m_val == uninitialized_probability) return m_quality == profile_guessed; else if (m_quality < profile_guessed) @@ -786,8 +787,7 @@ public: /* Return false if profile_count is bogus. */ bool verify () const { - gcc_checking_assert (profile_guessed_local <= m_quality - && m_quality <= profile_precise); + gcc_checking_assert (m_quality != profile_uninitialized); return m_val != uninitialized_count || m_quality == profile_guessed_local; }
On 01/19/2018 04:08 PM, Martin Liška wrote: > On 01/19/2018 02:21 PM, Tom de Vries wrote: >> How about keeping profile_uninitialized at the zero value location and asserting m_quality != profile_uninitialized ? >> >> Thanks, >> - Tom > > Yes, that would be possible. > > Can you please test that the patch does not generate warnings? > Confirmed, the warnings are gone. Thanks, - Tom > I'm running regression tests. > > Martin > > > 0001-Fix-profile_quality-sanity-check.patch > > > From e1159c2404947f675200efc4476e7e0994b81101 Mon Sep 17 00:00:00 2001 > From: marxin <mliska@suse.cz> > Date: Fri, 19 Jan 2018 15:27:40 +0100 > Subject: [PATCH] Fix profile_quality sanity check. > > gcc/ChangeLog: > > 2018-01-18 Martin Liska <mliska@suse.cz> > > * profile-count.h (enum profile_quality): Add > profile_uninitialized as the first value. Do not number values > as they are zero based. > (profile_count::verify): Update sanity check. > (profile_probability::verify): Likewise. > --- > gcc/profile-count.h | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/gcc/profile-count.h b/gcc/profile-count.h > index 7a43917ebbc..828d6d0ee4b 100644 > --- a/gcc/profile-count.h > +++ b/gcc/profile-count.h > @@ -26,34 +26,36 @@ struct function; > /* Quality of the profile count. Because gengtype does not support enums > inside of classes, this is in global namespace. */ > enum profile_quality { > + /* Uninitialized value. */ > + profile_uninitialized, > /* Profile is based on static branch prediction heuristics and may > or may not match reality. It is local to function and can not be compared > inter-procedurally. Never used by probabilities (they are always local). > */ > - profile_guessed_local = 1, > + profile_guessed_local, > /* Profile was read by feedback and was 0, we used local heuristics to guess > better. This is the case of functions not run in profile fedback. > Never used by probabilities. */ > - profile_guessed_global0 = 2, > + profile_guessed_global0, > > /* Same as profile_guessed_global0 but global count is adjusted 0. */ > - profile_guessed_global0adjusted = 3, > + profile_guessed_global0adjusted, > > /* Profile is based on static branch prediction heuristics. It may or may > not reflect the reality but it can be compared interprocedurally > (for example, we inlined function w/o profile feedback into function > with feedback and propagated from that). > Never used by probablities. */ > - profile_guessed = 4, > + profile_guessed, > /* Profile was determined by autofdo. */ > - profile_afdo = 5, > + profile_afdo, > /* Profile was originally based on feedback but it was adjusted > by code duplicating optimization. It may not precisely reflect the > particular code path. */ > - profile_adjusted = 6, > + profile_adjusted, > /* Profile was read from profile feedback or determined by accurate static > method. */ > - profile_precise = 7 > + profile_precise > }; > > /* The base value for branch probability notes and edge probabilities. */ > @@ -505,8 +507,7 @@ public: > /* Return false if profile_probability is bogus. */ > bool verify () const > { > - gcc_checking_assert (profile_guessed_local <= m_quality > - && m_quality <= profile_precise); > + gcc_checking_assert (m_quality != profile_uninitialized); > if (m_val == uninitialized_probability) > return m_quality == profile_guessed; > else if (m_quality < profile_guessed) > @@ -786,8 +787,7 @@ public: > /* Return false if profile_count is bogus. */ > bool verify () const > { > - gcc_checking_assert (profile_guessed_local <= m_quality > - && m_quality <= profile_precise); > + gcc_checking_assert (m_quality != profile_uninitialized); > return m_val != uninitialized_count || m_quality == profile_guessed_local; > } > >
diff --git a/gcc/profile-count.h b/gcc/profile-count.h index 7a43917ebbc..e899963118b 100644 --- a/gcc/profile-count.h +++ b/gcc/profile-count.h @@ -26,34 +26,36 @@ struct function; /* Quality of the profile count. Because gengtype does not support enums inside of classes, this is in global namespace. */ enum profile_quality { + /* Uninitialized value. */ + profile_uninitialized, /* Profile is based on static branch prediction heuristics and may or may not match reality. It is local to function and can not be compared inter-procedurally. Never used by probabilities (they are always local). */ - profile_guessed_local = 1, + profile_guessed_local, /* Profile was read by feedback and was 0, we used local heuristics to guess better. This is the case of functions not run in profile fedback. Never used by probabilities. */ - profile_guessed_global0 = 2, + profile_guessed_global0, /* Same as profile_guessed_global0 but global count is adjusted 0. */ - profile_guessed_global0adjusted = 3, + profile_guessed_global0adjusted, /* Profile is based on static branch prediction heuristics. It may or may not reflect the reality but it can be compared interprocedurally (for example, we inlined function w/o profile feedback into function with feedback and propagated from that). Never used by probablities. */ - profile_guessed = 4, + profile_guessed, /* Profile was determined by autofdo. */ - profile_afdo = 5, + profile_afdo, /* Profile was originally based on feedback but it was adjusted by code duplicating optimization. It may not precisely reflect the particular code path. */ - profile_adjusted = 6, + profile_adjusted, /* Profile was read from profile feedback or determined by accurate static method. */ - profile_precise = 7 + profile_precise }; /* The base value for branch probability notes and edge probabilities. */