Re: [PATCH] PR tree-optimization/57371: Optimize (float)i == 16777222.0f sometimes.

[Jeff Law <jeffreyalaw@gmail.com>]

On 7/28/24 6:01 AM, Roger Sayle wrote:
> 
> This patch improves the tree-level optimization of (fptype)ivar != CST
> in match.pd (historically tracked under PR 57371).  Joseph Myers'
> description in comment #1 provides an excellent overview of the issues,
> that historically it's the trapping behaviour of (fptype)ivar conversion
> that is the primary concern, which is why the current code in match.pd
> checks fmt.can_represent_integral_type_p (itype).  The first of the
> improvements with this patch is to check flag_trapping_math to control
> whether FP_OVERFLOW/FP_INEXACT needs to be preserved, and to use
> ranger to determine whether the bounds on ivar confirm that these
> traps aren't possible.  For example, the expression (int)var & 15
> can't overflow conversion to IEEE float, even though the type of a
> 32-bit int could potentially overflow the significant of a 32-bit
> float.
> 
> The next of the optimizations concern checking whether the comparison
> against CST is unambiguous allowing it to be replaced with a integer
> comparison.  For reference, consider the table below which shows the
> default conversion of integers to IEEE 32-bit float.
> 
> (float)16777211 => 16777211.0f;
> (float)16777212 => 16777212.0f;
> (float)16777213 => 16777213.0f;
> (float)16777214 => 16777214.0f;
> (float)16777215 => 16777215.0f;
> (float)16777216 => 16777216.0f;
> (float)16777217 => 16777216.0f;  // rounded
> (float)16777218 => 16777218.0f;
> (float)16777219 => 16777220.0f;  // rounded
> (float)16777220 => 16777220.0f;
> (float)16777221 => 16777220.0f;  // rounded
> (float)16777222 => 16777222.0f;
> (float)16777223 => 16777224.0f;  // rounded
> (float)16777224 => 16777224.0f;
> (float)16777225 => 16777224.0f;  // rounded
> (float)16777226 => 16777226.0f;
> 
> Observe that it's safe to optimize (float)i == 16777212.0f to the
> equivalent i == 16777212 (as this is the only integer that can
> convert to that floating point constant), but that it's unsafe to
> optimize (float)i == 16777220.0f, as with default rounding there
> are three possible integer values that FLOAT_EXPR to 16777220.0f.
> The pragmatic check used in this patch is to confirm that (float)(i-1)
> and (float)(i+1) are both distinct from (float)i before simplifying
> the comparison to an integer-typed comparison.
> 
> Finally, this patch also handles non-default rounding modes.
> In the table above, it's safe to optimize (float)i == 16777222.0f
> in IEEE's default rounding mode, but not in all FP rounding modes.
> This eventuality is handled by testing whether the (float)i, the
> (float)(i-1) and the (float)(i+1) are all exactly rounded when
> -frounding-math is specified.
> 
> 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?  If the testcases need to be
> tweaked for non-IEEE targets (the transformations themselves should
> be portable to VAX and IBM floating point formats) hopefully that
> can be done as follow-up patches by folks with the appropriate
> effective-targets?
> 
> 
> 2024-07-28  Roger Sayle  <roger@nextmovesoftware.com>
> 
> gcc/ChangeLog
>          PR tree-optimization/57371
>          * fold-const.cc (fold_cmp_float_cst_p): New helper function.
>          * fold-const.h (fold_cmp_float_cst_p): Prototype here.
>          * match.pd ((FTYPE) N CMP CST): Use ranger to determine
>          whether value is exactly representable by floating point type,
>          and check flag_trapping_math if not.  Use the new helper
>          fold_cmp_float_cst_p to check that transformation to an integer
>          comparison is safe.
> 
> gcc/testsuite/ChangeLog
>          PR tree-optimization/57371
>          * c-c++-common/pr57371-6.c: New test case.
>          * c-c++-common/pr57371-7.c: Likewise.
>          * c-c++-common/pr57371-8.c: Likewise.
>          * c-c++-common/pr57371-9.c: Likewise.
>          * c-c++-common/pr57371-10.c: Likewise.
Nice.  I was a bit concerned about using Ranger in match.pd as match.pd 
can be used for GENERIC as well as GIMPLE.  But it looks like you 
handled that reasonably.  Similarly for the other FP formats.

As you note, I wouldn't be terribly surprised if the other FP formats 
need testsuite adjustments.  I'd hoped we had a target selector, but we 
don't.

Does it make sense to use "add_options_for_ieee" to conditionally add 
the necessary target options in the tests?   It only affects alpha, sh & 
rx, so it's unlikely to have been needed in your testing.

Jeff