* [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back. @ 2022-10-25 21:01 Aldy Hernandez 2022-10-26 7:57 ` Aldy Hernandez 2022-10-27 22:45 ` Jeff Law 0 siblings, 2 replies; 6+ messages in thread From: Aldy Hernandez @ 2022-10-25 21:01 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, GCC patches, Andrew MacLeod, Aldy Hernandez [Richi/Jakub/FP experts, does this sound like the right solution, or am I missing some subtle IPA/inlining issue?] The problem here is that we're inlining a global range with NANs into a function that has been tagged with __attribute__((optimize ("-ffinite-math-only"))). As the global range is copied from SSA_NAME_RANGE_INFO, its NAN bits are copied, which then cause frange::verify_range() to fail a sanity check making sure no NANs creep in when !HONOR_NANS. I think what we should do is nuke the NAN bits as we're restoring the global range. For that matter, if we use the frange constructor, everything except that NAN sign will be done automatically, including dropping INFs to the min/max representable range when appropriate. PR tree-optimization/107394 gcc/ChangeLog: * value-range-storage.cc (frange_storage_slot::get_frange): Use frange constructor. gcc/testsuite/ChangeLog: * gcc.dg/tree-ssa/pr107394.c: New test. --- gcc/testsuite/gcc.dg/tree-ssa/pr107394.c | 22 ++++++++++++++++++++++ gcc/value-range-storage.cc | 18 +++++++----------- 2 files changed, 29 insertions(+), 11 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr107394.c diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c b/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c new file mode 100644 index 00000000000..0e1e5ac40ce --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c @@ -0,0 +1,22 @@ +// { dg-do compile } +// { dg-options "-O2" } + +static double +quux (double x) +{ + return __builtin_fabs (x); +} + +__attribute__ ((flatten, optimize ("-ffinite-math-only"))) static int +bar (int *p) +{ + *p = quux (0.0); + + return 0; +} + +void +foo (int *p) +{ + (void) bar (p); +} diff --git a/gcc/value-range-storage.cc b/gcc/value-range-storage.cc index 6e054622830..b660102e064 100644 --- a/gcc/value-range-storage.cc +++ b/gcc/value-range-storage.cc @@ -261,17 +261,13 @@ frange_storage_slot::get_frange (frange &r, tree type) const { gcc_checking_assert (r.supports_type_p (type)); - r.set_undefined (); - r.m_kind = m_kind; - r.m_type = type; - r.m_min = m_min; - r.m_max = m_max; - r.m_pos_nan = m_pos_nan; - r.m_neg_nan = m_neg_nan; - r.normalize_kind (); - - if (flag_checking) - r.verify_range (); + // Use the constructor because it will canonicalize the range. + r = frange (type, m_min, m_max, m_kind); + + // The constructor will set the NAN bits for HONOR_NANS, but we must + // make sure to set the NAN sign if known. + if (HONOR_NANS (type) && (m_pos_nan ^ m_neg_nan) == 1) + r.update_nan (m_neg_nan); } bool -- 2.37.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back. 2022-10-25 21:01 [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back Aldy Hernandez @ 2022-10-26 7:57 ` Aldy Hernandez 2022-10-27 22:45 ` Jeff Law 1 sibling, 0 replies; 6+ messages in thread From: Aldy Hernandez @ 2022-10-26 7:57 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, GCC patches, Andrew MacLeod [-- Attachment #1: Type: text/plain, Size: 2901 bytes --] Updated tested patch. Aldy On Tue, Oct 25, 2022 at 11:02 PM Aldy Hernandez <aldyh@redhat.com> wrote: > > [Richi/Jakub/FP experts, does this sound like the right solution, or am I > missing some subtle IPA/inlining issue?] > > The problem here is that we're inlining a global range with NANs into > a function that has been tagged with __attribute__((optimize > ("-ffinite-math-only"))). As the global range is copied from > SSA_NAME_RANGE_INFO, its NAN bits are copied, which then cause > frange::verify_range() to fail a sanity check making sure no NANs > creep in when !HONOR_NANS. > > I think what we should do is nuke the NAN bits as we're restoring the > global range. For that matter, if we use the frange constructor, > everything except that NAN sign will be done automatically, including > dropping INFs to the min/max representable range when appropriate. > > PR tree-optimization/107394 > > gcc/ChangeLog: > > * value-range-storage.cc (frange_storage_slot::get_frange): Use > frange constructor. > > gcc/testsuite/ChangeLog: > > * gcc.dg/tree-ssa/pr107394.c: New test. > --- > gcc/testsuite/gcc.dg/tree-ssa/pr107394.c | 22 ++++++++++++++++++++++ > gcc/value-range-storage.cc | 18 +++++++----------- > 2 files changed, 29 insertions(+), 11 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr107394.c > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c b/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c > new file mode 100644 > index 00000000000..0e1e5ac40ce > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c > @@ -0,0 +1,22 @@ > +// { dg-do compile } > +// { dg-options "-O2" } > + > +static double > +quux (double x) > +{ > + return __builtin_fabs (x); > +} > + > +__attribute__ ((flatten, optimize ("-ffinite-math-only"))) static int > +bar (int *p) > +{ > + *p = quux (0.0); > + > + return 0; > +} > + > +void > +foo (int *p) > +{ > + (void) bar (p); > +} > diff --git a/gcc/value-range-storage.cc b/gcc/value-range-storage.cc > index 6e054622830..b660102e064 100644 > --- a/gcc/value-range-storage.cc > +++ b/gcc/value-range-storage.cc > @@ -261,17 +261,13 @@ frange_storage_slot::get_frange (frange &r, tree type) const > { > gcc_checking_assert (r.supports_type_p (type)); > > - r.set_undefined (); > - r.m_kind = m_kind; > - r.m_type = type; > - r.m_min = m_min; > - r.m_max = m_max; > - r.m_pos_nan = m_pos_nan; > - r.m_neg_nan = m_neg_nan; > - r.normalize_kind (); > - > - if (flag_checking) > - r.verify_range (); > + // Use the constructor because it will canonicalize the range. > + r = frange (type, m_min, m_max, m_kind); > + > + // The constructor will set the NAN bits for HONOR_NANS, but we must > + // make sure to set the NAN sign if known. > + if (HONOR_NANS (type) && (m_pos_nan ^ m_neg_nan) == 1) > + r.update_nan (m_neg_nan); > } > > bool > -- > 2.37.3 > [-- Attachment #2: 0001-PR-tree-optimization-107394-Canonicalize-global-fran.patch --] [-- Type: text/x-patch, Size: 2989 bytes --] From 89eaccbc4b1e5ff7fea2eb4e3fb5870c6ca4160b Mon Sep 17 00:00:00 2001 From: Aldy Hernandez <aldyh@redhat.com> Date: Tue, 25 Oct 2022 22:44:51 +0200 Subject: [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back. The problem here is that we're inlining a global range with NANs into a function that has been tagged with __attribute__((optimize ("-ffinite-math-only"))). As the global range is copied from SSA_NAME_RANGE_INFO, its NAN bits are copied, which then cause frange::verify_range() to fail a sanity check making sure no NANs creep in when !HONOR_NANS. I think what we should do is nuke the NAN bits as we're restoring the global range. For that matter, if we use the frange constructor, everything except that NAN sign will be done automatically, including dropping INFs to the min/max representable range when appropriate. PR tree-optimization/107394 gcc/ChangeLog: * value-range-storage.cc (frange_storage_slot::get_frange): Use frange constructor. gcc/testsuite/ChangeLog: * gcc.dg/tree-ssa/pr107394.c: New test. --- gcc/testsuite/gcc.dg/tree-ssa/pr107394.c | 22 ++++++++++++++++ gcc/value-range-storage.cc | 33 ++++++++++++++++-------- 2 files changed, 44 insertions(+), 11 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr107394.c diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c b/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c new file mode 100644 index 00000000000..0e1e5ac40ce --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr107394.c @@ -0,0 +1,22 @@ +// { dg-do compile } +// { dg-options "-O2" } + +static double +quux (double x) +{ + return __builtin_fabs (x); +} + +__attribute__ ((flatten, optimize ("-ffinite-math-only"))) static int +bar (int *p) +{ + *p = quux (0.0); + + return 0; +} + +void +foo (int *p) +{ + (void) bar (p); +} diff --git a/gcc/value-range-storage.cc b/gcc/value-range-storage.cc index 6e054622830..462447ba250 100644 --- a/gcc/value-range-storage.cc +++ b/gcc/value-range-storage.cc @@ -261,17 +261,28 @@ frange_storage_slot::get_frange (frange &r, tree type) const { gcc_checking_assert (r.supports_type_p (type)); - r.set_undefined (); - r.m_kind = m_kind; - r.m_type = type; - r.m_min = m_min; - r.m_max = m_max; - r.m_pos_nan = m_pos_nan; - r.m_neg_nan = m_neg_nan; - r.normalize_kind (); - - if (flag_checking) - r.verify_range (); + // Handle explicit NANs. + if (m_kind == VR_NAN) + { + if (HONOR_NANS (type)) + { + if (m_pos_nan && m_neg_nan) + r.set_nan (type); + else + r.set_nan (type, m_neg_nan); + } + else + r.set_undefined (); + return; + } + + // Use the constructor because it will canonicalize the range. + r = frange (type, m_min, m_max, m_kind); + + // The constructor will set the NAN bits for HONOR_NANS, but we must + // make sure to set the NAN sign if known. + if (HONOR_NANS (type) && (m_pos_nan ^ m_neg_nan) == 1) + r.update_nan (m_neg_nan); } bool -- 2.37.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back. 2022-10-25 21:01 [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back Aldy Hernandez 2022-10-26 7:57 ` Aldy Hernandez @ 2022-10-27 22:45 ` Jeff Law 2022-10-28 6:48 ` Richard Biener 1 sibling, 1 reply; 6+ messages in thread From: Jeff Law @ 2022-10-27 22:45 UTC (permalink / raw) To: Aldy Hernandez, Richard Biener; +Cc: Jakub Jelinek, GCC patches On 10/25/22 15:01, Aldy Hernandez via Gcc-patches wrote: > [Richi/Jakub/FP experts, does this sound like the right solution, or am I > missing some subtle IPA/inlining issue?] > > The problem here is that we're inlining a global range with NANs into > a function that has been tagged with __attribute__((optimize > ("-ffinite-math-only"))). As the global range is copied from > SSA_NAME_RANGE_INFO, its NAN bits are copied, which then cause > frange::verify_range() to fail a sanity check making sure no NANs > creep in when !HONOR_NANS. > > I think what we should do is nuke the NAN bits as we're restoring the > global range. For that matter, if we use the frange constructor, > everything except that NAN sign will be done automatically, including > dropping INFs to the min/max representable range when appropriate. > > PR tree-optimization/107394 > > gcc/ChangeLog: > > * value-range-storage.cc (frange_storage_slot::get_frange): Use > frange constructor. > > gcc/testsuite/ChangeLog: > > * gcc.dg/tree-ssa/pr107394.c: New test. The other approach would be to disabling inlining in this case due to an unsafe attribute mismatch, but we're not currently doing much sanity checking in this space and it might be a huge can of worms. I'm inclined to ACK, but give Jakub and Richi until Monday to chime in first. jeff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back. 2022-10-27 22:45 ` Jeff Law @ 2022-10-28 6:48 ` Richard Biener 2022-10-28 6:50 ` Richard Biener 2022-10-28 8:23 ` Aldy Hernandez 0 siblings, 2 replies; 6+ messages in thread From: Richard Biener @ 2022-10-28 6:48 UTC (permalink / raw) To: Jeff Law; +Cc: Aldy Hernandez, Jakub Jelinek, GCC patches On Fri, Oct 28, 2022 at 12:45 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > On 10/25/22 15:01, Aldy Hernandez via Gcc-patches wrote: > > [Richi/Jakub/FP experts, does this sound like the right solution, or am I > > missing some subtle IPA/inlining issue?] > > > > The problem here is that we're inlining a global range with NANs into > > a function that has been tagged with __attribute__((optimize > > ("-ffinite-math-only"))). As the global range is copied from > > SSA_NAME_RANGE_INFO, its NAN bits are copied, which then cause > > frange::verify_range() to fail a sanity check making sure no NANs > > creep in when !HONOR_NANS. > > > > I think what we should do is nuke the NAN bits as we're restoring the > > global range. For that matter, if we use the frange constructor, > > everything except that NAN sign will be done automatically, including > > dropping INFs to the min/max representable range when appropriate. > > > > PR tree-optimization/107394 > > > > gcc/ChangeLog: > > > > * value-range-storage.cc (frange_storage_slot::get_frange): Use > > frange constructor. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/tree-ssa/pr107394.c: New test. > > The other approach would be to disabling inlining in this case due to an > unsafe attribute mismatch, but we're not currently doing much sanity > checking in this space and it might be a huge can of worms. I'm > inclined to ACK, but give Jakub and Richi until Monday to chime in first. We are actually quite careful in this regard but maybe our reasoning is wrong. We are allowing inlining of -fno-finite-math-only into -ffinite-math-only code but not the other way around. On the actual patch I think that ranges with Inf/NaNs should be always treated as "valid", the optimization to trim them with certain options is optimization and thus optional. So IMHO having verify_range ICE on NaNs isn't correct? That said, the patch is in line with what we do elsewhere at the moment, so I guess OK. Richard. > > jeff > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back. 2022-10-28 6:48 ` Richard Biener @ 2022-10-28 6:50 ` Richard Biener 2022-10-28 8:23 ` Aldy Hernandez 1 sibling, 0 replies; 6+ messages in thread From: Richard Biener @ 2022-10-28 6:50 UTC (permalink / raw) To: Jeff Law; +Cc: Aldy Hernandez, Jakub Jelinek, GCC patches On Fri, Oct 28, 2022 at 8:48 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Fri, Oct 28, 2022 at 12:45 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > > > > On 10/25/22 15:01, Aldy Hernandez via Gcc-patches wrote: > > > [Richi/Jakub/FP experts, does this sound like the right solution, or am I > > > missing some subtle IPA/inlining issue?] > > > > > > The problem here is that we're inlining a global range with NANs into > > > a function that has been tagged with __attribute__((optimize > > > ("-ffinite-math-only"))). As the global range is copied from > > > SSA_NAME_RANGE_INFO, its NAN bits are copied, which then cause > > > frange::verify_range() to fail a sanity check making sure no NANs > > > creep in when !HONOR_NANS. > > > > > > I think what we should do is nuke the NAN bits as we're restoring the > > > global range. For that matter, if we use the frange constructor, > > > everything except that NAN sign will be done automatically, including > > > dropping INFs to the min/max representable range when appropriate. > > > > > > PR tree-optimization/107394 > > > > > > gcc/ChangeLog: > > > > > > * value-range-storage.cc (frange_storage_slot::get_frange): Use > > > frange constructor. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.dg/tree-ssa/pr107394.c: New test. > > > > The other approach would be to disabling inlining in this case due to an > > unsafe attribute mismatch, but we're not currently doing much sanity > > checking in this space and it might be a huge can of worms. I'm > > inclined to ACK, but give Jakub and Richi until Monday to chime in first. > > We are actually quite careful in this regard but maybe our reasoning > is wrong. We are allowing inlining of -fno-finite-math-only into > -ffinite-math-only code but not the other way around. > > On the actual patch I think that ranges with Inf/NaNs should be always > treated as "valid", the optimization to trim them with certain options > is optimization and thus optional. So IMHO having verify_range ICE > on NaNs isn't correct? Just to make a point here - in functions with -ffinite-math-only in effect volatile double x = __builtin_nan(""); will still have a literal NaN in the IL and that's not invalid GIMPLE. You cannot assume that no NaNs appear with -ffinite-math-only, you just don't need to specially are about preserving them. > That said, the patch is in line with what we do elsewhere at the moment, > so I guess OK. > > Richard. > > > > > jeff > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back. 2022-10-28 6:48 ` Richard Biener 2022-10-28 6:50 ` Richard Biener @ 2022-10-28 8:23 ` Aldy Hernandez 1 sibling, 0 replies; 6+ messages in thread From: Aldy Hernandez @ 2022-10-28 8:23 UTC (permalink / raw) To: Richard Biener; +Cc: Jeff Law, Jakub Jelinek, GCC patches, MacLeod, Andrew [-- Attachment #1: Type: text/plain, Size: 2625 bytes --] On Fri, Oct 28, 2022, 08:49 Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Oct 28, 2022 at 12:45 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > > > > On 10/25/22 15:01, Aldy Hernandez via Gcc-patches wrote: > > > [Richi/Jakub/FP experts, does this sound like the right solution, or > am I > > > missing some subtle IPA/inlining issue?] > > > > > > The problem here is that we're inlining a global range with NANs into > > > a function that has been tagged with __attribute__((optimize > > > ("-ffinite-math-only"))). As the global range is copied from > > > SSA_NAME_RANGE_INFO, its NAN bits are copied, which then cause > > > frange::verify_range() to fail a sanity check making sure no NANs > > > creep in when !HONOR_NANS. > > > > > > I think what we should do is nuke the NAN bits as we're restoring the > > > global range. For that matter, if we use the frange constructor, > > > everything except that NAN sign will be done automatically, including > > > dropping INFs to the min/max representable range when appropriate. > > > > > > PR tree-optimization/107394 > > > > > > gcc/ChangeLog: > > > > > > * value-range-storage.cc (frange_storage_slot::get_frange): Use > > > frange constructor. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.dg/tree-ssa/pr107394.c: New test. > > > > The other approach would be to disabling inlining in this case due to an > > unsafe attribute mismatch, but we're not currently doing much sanity > > checking in this space and it might be a huge can of worms. I'm > > inclined to ACK, but give Jakub and Richi until Monday to chime in first. > > We are actually quite careful in this regard but maybe our reasoning > is wrong. We are allowing inlining of -fno-finite-math-only into > -ffinite-math-only code but not the other way around. > > On the actual patch I think that ranges with Inf/NaNs should be always > treated as "valid", the optimization to trim them with certain options > is optimization and thus optional. So IMHO having verify_range ICE > on NaNs isn't correct? > That was my gut feeling as well, but the assert has caught real issues such as this one. Also, in your example down thread, we would drop the explicit NAN to UNDEFINED if expressed as a range (as agreed earlier this cycle). So we won't ICE...since a range with NAN will never get built. The assert is there to keep NANs from sneaking in. However, if you still think it's incorrect I'm happy to remove it. > That said, the patch is in line with what we do elsewhere at the moment, > so I guess OK. > Thanks. Aldy > Richard. > > > > > jeff > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-28 8:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-25 21:01 [PATCH] [PR tree-optimization/107394] Canonicalize global franges as they are read back Aldy Hernandez 2022-10-26 7:57 ` Aldy Hernandez 2022-10-27 22:45 ` Jeff Law 2022-10-28 6:48 ` Richard Biener 2022-10-28 6:50 ` Richard Biener 2022-10-28 8:23 ` Aldy Hernandez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).