On Fri, Jul 17, 2026 at 8:30 PM Roger Sayle <[email protected]> wrote: > > > > > Am 17.07.2026 um 17:40 schrieb Roger Sayle > > <[email protected]>: > > > Hi Richard, > > > > > >> From: Richard Biener <[email protected]> > > >> Sent: 17 July 2026 10:46 > > >> To: Roger Sayle <[email protected]> > > >> Cc: GCC Patches <[email protected]>; Andrew Pinski > > >> <[email protected]> > > >> Subject: Re: [PATCH] PR tree-optimization/126242: Check range is > > >> defined to avoid ICE. > > >> > > >>> On Wed, Jul 15, 2026 at 10:05 PM Roger Sayle > > >>> <[email protected]> > > >>> wrote: > > >>> > > >>> > > >>> This patch resolves PR tree-opt/126242, an unanticipated interaction > > >>> between the two recent (float)i == 1.0 patches to match.pd. The > > >>> issue is that value range information is getting queried in > > >>> circumstances (on paths) where we've failed to initialize the range > > >>> and/or ranger has failed to bound the value. The correction below > > >>> fixes this in two > > >>> ways: initialize the range information in more cases, and check that > > >>> the range has been successfully initialized before using it. > > >>> > > >>> The motivation/benefit for the first approach is seen in the example: > > >>> > > >>> unsigned char t = x & 63; > > >>> return (float)t > 100.0; > > >>> > > >>> Previously, because unsigned char can be safely represented in a > > >>> float we'd use the bounds [0,255], and transform this to t > 100. > > >>> Obviously, there's benefit in using ranger to reduce the range to > > >>> [0,63], even when the integer type fits the floating point type, > > >>> allowing the above expression to be simplified even further to false. > > >>> [Admittedly, this gets cleaned up in later passes/optimizations, but > > >>> this shows a potential benefit rather than just an inefficiency for > > >>> safety's sake]. > > >>> > > >>> > > >>> This patch has been tested on x86_64-pc-linux-gnu with make > > >>> bootstrap and make -k check, both with and without > > >>> --target_board=unix{-m32} with no new failures. Ok for mainline? > > >> > > >> I'm trying to follow the logic of itype_ok and value_ok. It might be > > >> easier to follow if hoisting i{min,max}_val out of the if > > >> (!exception_p) like > > >> > > >> wide_int imin_val = wi::min_value (itype); > > >> wide_int imax_val = wi::max_value (itype); > > >> bool value_ok = fmt.can_represent_integral_type_p (itype); #if GIMPLE > > >> if (!value_ok > > >> && gimple_match_range_of_expr (vr, @0, @2) > > >> && fmt.can_represent_range_value_p (&vr)) > > >> { > > >> imin_val = vr.lower_bound (); > > >> imax_val = vr.upper_bound (); > > >> value_ok = true; > > >> } > > >> #endif > > >> > > >> and elide itype_ok? > > >> > > >> OK with that change. > > > > > > No, the original fix is better. In your suggestion above, the range > > > is only calculated if the type fits, and then only used if the range fits. > > > > I read it the opposite way (but maybe I am missing a UNDEFINED check on the > > range) > > The underlying problem is with the clause: > > >> if (!value_ok > >> && gimple_match_range_of_expr (vr, @0, @2) > >> && fmt.can_represent_range_value_p (&vr)) > >> ... > > Here gimple_match_range_of_expr isn't a simple predicate, but > has side-effects, in fact this is the place where vr gets initialized. > > Hence, in the idiom above, vr is only initialized if value_ok is false. > Likewise, imin_val and imax_val only get updated to make use of > the calculated range information if fmt.can_represent_range_value_p > returns true. > > This is why Eikansh's patch used both itype_ok and value_ok, so that > one could be (ab)used to keep track of whether the range had been > initialized. > > So a better idiom looks like: > > // Always initialize vr (if possible) > if (gimple_match_range_of_expr (vr, @0, @2)) { > // Use improved bounds now we have them. > imin_val = vr.lower(); > imax_val = vr.upper(); > // Carry on with the original logic > if (!value_ok && fmt.can_represent_range_value_p (&vr)) > value_ok = true; > > It's the use of C++ pass by reference, that makes it unclear when/where > vr ever gets initialized. > > static inline bool > gimple_match_range_of_expr (vrange &r, ...) > > > Of course, the very minimal fix is to just use !vr.undefined_p () to confirm > the range is valid. Making sure the range's validity is not conditional on > value_ok (or -Ofast), makes the logic easier to follow, as well as catching > a missed optimization. No-one in future needs to puzzle why/how is this > variable getting set with one set of options, but not another. It's only > because gfortran can make gimple_match_range_of_expr return false, > i.e. not initialize the range even though value_ok/itype_ok is false, that > we can see the flawed/fragile logic.
What it definitely shows is that the code lacks comments, looking _again_ it seems that only in the flag_trapping_math case we need either fmt.can_represent_integral_type_p or fmt.can_represent_range_value_p but without flag_trapping_math we want to make use of a narrow known range as well. IMO doing away with fmt.can_represent_integral_type_p and always using a range, with the fallback Andrew proposes, combining "exception_p" into the if like /* ... comment ... */ (if (!flag_trapping_math || fmt.can_represent_range_value_p (&vr)) makes things more easily understandable. My compliant was that your patch seemed to make it even less comprehensible than it was. Thanks, Richard. > > > > See the text of my explanation (especially the example) for how things > > > can be improved (for -Ofast where value_ok is irrelevant). > > > > > > But I agree there's no point in having both itype_ok and value_ok. > > > > > > My original patch from 2024 didn’t have any of these problems. > > > > > >>> 2026-07-15 Roger Sayle <[email protected]> > > >>> > > >>> gcc/ChangeLog > > >>> PR tree-optimization/126242 > > >>> * match.pd ((FTYPE) N CMP CST): Always attempt to initialize > > >>> value range information. Check undefined_p before using range > > >>> bounds. > > >>> > > >>> gcc/testsuite/ChangeLog > > >>> PR tree-optimization/126242 > > >>> * gfortran.dg/pr126242.f90: New reduced test case. > > >>> * gfortran.dg/pr41928-2.f90: Also compile pr41928.f90 with > > >>> -Ofast. > > >>> > > >>> > > >>> Thanks in advance (and my apologies for any inconvenience), Roger > > >>> -- >
