On Fri, Oct 28, 2022 at 8:48 AM Richard Biener <richard.guent...@gmail.com> wrote: > > On Fri, Oct 28, 2022 at 12:45 AM Jeff Law <jeffreya...@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 > >