Created attachment 25236 [details] Test aimed to reproduce the -ffast-math comparison bug This is a regression from clang-11 to clang-12, but could be considered normal if we consider -ffast-math as being "by design" unreliable w.r.t comparisons. However, this breaks the use of functions such as strtod(), which can be considered a more serious issue, because the "== HUGE_VAL" comparison is considered by the compiler impossible and statically optimized as false The attached program demonstrates the issue: $ clang-11 -O1 -ffast-math /tmp/ffast-math-strtod-overflow-condition-misoptimization-bug.c -o bug && ./bug bad value $ clang-12 -O1 -ffast-math /tmp/ffast-math-strtod-overflow-condition-misoptimization-bug.c -o bug && ./bug good value: inf Clang versions 12 and above now are not seeing HUGE_VAL anymore
This regressed (or was changed on purpose) with https://github.com/llvm/llvm-project/commit/7393d7574c0 ("[InstSimplify] fold fcmp with infinity constant using isKnownNeverInfinity"). The rest of the commit message explains the rationale: > This is a step towards trying to remove unnecessary FP compares > with infinity when compiling with -ffinite-math-only or similar. > I'm intentionally not checking FMF on the fcmp itself because > I'm assuming that will go away eventually. > The analysis part of this was added with rGcd481136 for use with > isKnownNeverNaN. Similarly, that could be an enhancement here to > get predicates like 'one' and 'ueq'. IIRC -ffast-math implies -ffinite-math-only? And since HUGE_VAL is most often defined as +Inf this automatically makes these comparisons fold at compile time. Sanjay, I guess that is the short story? :)
(In reply to Dimitry Andric from comment #1) > This regressed (or was changed on purpose) with > https://github.com/llvm/llvm-project/commit/7393d7574c0 ("[InstSimplify] > fold fcmp with infinity constant using isKnownNeverInfinity"). > > The rest of the commit message explains the rationale: > > > This is a step towards trying to remove unnecessary FP compares > > with infinity when compiling with -ffinite-math-only or similar. > > I'm intentionally not checking FMF on the fcmp itself because > > I'm assuming that will go away eventually. > > The analysis part of this was added with rGcd481136 for use with > > isKnownNeverNaN. Similarly, that could be an enhancement here to > > get predicates like 'one' and 'ueq'. > > IIRC -ffast-math implies -ffinite-math-only? And since HUGE_VAL is most > often defined as +Inf this automatically makes these comparisons fold at > compile time. > > Sanjay, I guess that is the short story? :) Yes - thanks for digging up the path here. The question of how to deal with finite-math-only and loaded/compared values is part of what's driving a current discussion about adding FP intrinsics to LLVM: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html https://lists.llvm.org/pipermail/llvm-dev/2021-September/152431.html https://lists.llvm.org/pipermail/llvm-dev/2021-September/152455.html I haven't grabbed the attachment yet, but if someone can simulate that on godbolt and/or post the unoptimized IR, that would be a good 1st step to see if we can work-around this some way.
Clang currently generates: %10 = call fast double @strtod(i8* getelementptr inbounds ([11 x i8], [11 x i8]* @.str, i64 0, i64 0), i8** null) #7, !dbg !33 The "fast" marking comes from clang. If we don't want to assume the result of strtod is non-inf, we probably should change it on the clang side.
(In reply to Eli Friedman from comment #3) > Clang currently generates: > > %10 = call fast double @strtod(i8* getelementptr inbounds ([11 x i8], [11 > x i8]* @.str, i64 0, i64 0), i8** null) #7, !dbg !33 > > The "fast" marking comes from clang. If we don't want to assume the result > of strtod is non-inf, we probably should change it on the clang side. Thanks! Currently, clang is setting the Builder's FMF and then that gets applied to anything that qualifies as an FPMathOperator. There are also some target-specific math intrinsics where we apply extra FMF on top of the Builder settings because loose semantics are implied in their definitions (eg, x86 vector FP reductions). An FPMathOperator includes any function that returns a value with an FPOrFPVectorTy(). So a fix in clang would have to distinguish between calls that should have FMF and others. I think we *do* want to apply FMF to arbitrary user calls that return an FP value, so that means we need to block FMF on some set of libcalls. Does something like this make sense? diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index 8548a9e75f4b..1a0ab85f0208 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -4857,8 +4857,15 @@ RValue CodeGenFunction::EmitCallExpr(const CallExpr *E, CGCallee callee = EmitCallee(E->getCallee()); if (callee.isBuiltin()) { - return EmitBuiltinExpr(callee.getBuiltinDecl(), callee.getBuiltinID(), - E, ReturnValue); + // Regardless of the builder's current fast-math mode and the definition of + // an FP operator, we do not want FMF applied to standard library calls + // ("strtod" for example). + unsigned ID = callee.getBuiltinID(); + CGBuilderTy::FastMathFlagGuard FMFGuard(Builder); + const char *HeaderName = getContext().BuiltinInfo.getHeaderName(ID); + if (HeaderName && !strcmp(HeaderName, "stdlib.h")) + Builder.getFastMathFlags().clear(); + return EmitBuiltinExpr(callee.getBuiltinDecl(), ID, E, ReturnValue); } if (callee.isPseudoDestructor()) {
Handling infinity coming out of C library code differently from infinity coming out of user code seems more likely to be confusing than helpful. If we're going to mess with the fast-math flags on calls, we should do it for all calls. Maybe with a allow-list of math library calls that we apply fast-math flags to.
That said, I'm not confident we want to change the behavior here. If the user writes -ffinite-math-only, and then tries to do stuff with infinity, that seems like the user's fault. Maybe we should warn.
I put an example based on this report in the latest cfe/llvm dev thread on this topic: https://lists.llvm.org/pipermail/cfe-dev/2021-September/068828.html First post in that thread here: https://lists.llvm.org/pipermail/cfe-dev/2021-September/068814.html
I'm confused about clang internals, but here's a proposal to add warnings: https://reviews.llvm.org/D109925