On Thu, 3 Oct 2024, Jennifer Schmitz wrote: > > > > On 1 Oct 2024, at 14:27, Richard Biener <rguent...@suse.de> wrote: > > > > External email: Use caution opening links or attachments > > > > > > On Tue, 1 Oct 2024, Jennifer Schmitz wrote: > > > >> This patch implements 4 rules for logarithmic identities in match.pd > >> under -funsafe-math-optimizations: > >> 1) logN(1.0/a) -> -logN(a). This avoids the division instruction. > >> 2) logN(C/a) -> logN(C) - logN(a), where C is a real constant. Same as 1). > >> 3) logN(a) + logN(b) -> logN(a*b). This reduces the number of calls to > >> log function. > >> 4) logN(a) - logN(b) -> logN(a/b). Same as 4). > >> Tests were added for float, double, and long double. > >> > >> The patch was bootstrapped and regtested on aarch64-linux-gnu and > >> x86_64-linux-gnu, no regression. > >> Additionally, SPEC 2017 fprate was run. While the transform does not seem > >> to be triggered, we also see no non-noise impact on performance. > >> OK for mainline? > > > > Since log can set errno we have the builtins affect global memory and > > thus have VDEFs, this posses issues for match.pd which does not assign > > new VDEFs upon materializing the result, esp. for the case where > > you duplicate a call. There's a similar issue for -frounding-math > > where intermediate FP status changes can be lost. match.pd simply > > follows the SSA use-def chains without regarding memory side-effects. > > > > The transforms are guarded by flag_unsafe_math_optimizations but here > > I think we need !HONOR_SIGN_DEPENDENT_ROUNDING, !flag_trapping_math > > (exception state might be different for logN(a) - logN(b) -> logN(a/b), > > at least WRT INEXACT?), and !flag_errno_math (because of the VDEFs). > > > > + /* Simplify logN(C/a) into logN(C)-logN(a). */ > > + (simplify > > + (logs (rdiv:s REAL_CST@0 @1)) > > + (minus (logs @0) (logs @1))) > > > > I think you want > > > > (minus (logs! @0) (logs @1)) > > > > here to make sure we constant-fold. > > > > + (simplify > > + (minus (logs:s @0) (logs:s @1)) > > + (logs (rdiv @0 @1)))) > > > > I think that's somewhat dangerous for @1 == 0 given log for > > zero arg results in -HUGE_VAL but a FP division by gives a NaN. > > I'm not exactly sure whether !HONOR_INFINITIES && !HONOR_NANS > > is good enough here. > > > > Your testcases probably all trigger during GENERIC folding, > > bypassing the VDEF issue - you might want to try assigning > > the comparison operands to tempoaries to run into the actual > > issues. > Dear Richard, > Thanks for the review and suggesting the additional flags. I added > - !HONOR_SIGN_DEPENDENT_ROUNDING > - !flag_trapping_math > - !flag_errno_math > - !HONOR_INFINITIES > - !HONOR_NANS > as guard before the patterns. > Can we add anything else to account for HUGE_VAL or will !HONOR_INFINITIES && > !HONOR_NANS be enough? Or do you have a suggestion how I can check this? > I validated again on aarch64 and x86_64.
Can you change your patch attachment format to be at least text/plain and ideally just inline it or use git send-mail? This way reviewing is much easier. I'll quote it here for reference: (if (flag_unsafe_math_optimizations) [...] + (if (! HONOR_SIGN_DEPENDENT_ROUNDING (type) + && ! HONOR_NANS (type) && ! HONOR_INFINITIES (type) + && ! flag_trapping_math + && ! flag_errno_math) + (for logs (LOG LOG2 LOG10) + /* Simplify logN(1.0/a) into -logN(a). */ + (simplify + (logs (rdiv:s real_onep@0 @1)) + (negate (logs @1))) + + /* Simplify logN(C/a) into logN(C)-logN(a). */ + (simplify + (logs (rdiv:s REAL_CST@0 @1)) + (minus (logs! @0) (logs @1))) + + /* Simplify logN(a)+logN(b) into logN(a*b). */ + (simplify + (plus (logs:s @0) (logs:s @1)) + (logs (mult @0 @1))) + + /* Simplify logN(a)-logN(b) into logN(a/b). */ + (simplify + (minus (logs:s @0) (logs:s @1)) + (logs (rdiv @0 @1))))) I'm OK with the extra guards added but I'm also not a IEEE FP expert here. In the previous review I did mention the extra constraints for specific sub-patterns IIRC. As a general note we might want to implement a HONOR_ERRNO (combined_fn). Probably a bad name, builtin_can_set_errno (combined_fn) might be better, for example above I'm not sure whether all of log(), log2() and log10() can set errno and how we generally should handle errno when -fno-math-errno isn't given and GCC wasn't configured specifically to target for example glibc. For glibc it might be nice if we could tell it we're not interested in errno from math functions and in this way convey -fno-math-errno to it for example via a crtfastmath like mechanism (or by calling alternate entry points). So, the patch is OK in case Joseph doesn't have any comments during this week. Thanks, Richard.
