LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 51775 - -ffast-math breaks strtod() due to "== HUGE_VAL" considered impossible
Summary: -ffast-math breaks strtod() due to "== HUGE_VAL" considered impossible
Status: NEW
Alias: None
Product: clang
Classification: Unclassified
Component: C (show other bugs)
Version: 12.0
Hardware: PC Linux
: P normal
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-07 02:15 PDT by Xavier Roche
Modified: 2021-09-16 18:42 PDT (History)
9 users (show)

See Also:
Fixed By Commit(s):


Attachments
Test aimed to reproduce the -ffast-math comparison bug (345 bytes, text/x-csrc)
2021-09-07 02:15 PDT, Xavier Roche
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Xavier Roche 2021-09-07 02:15:54 PDT
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
Comment 1 Dimitry Andric 2021-09-07 10:56:06 PDT
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? :)
Comment 2 Sanjay Patel 2021-09-07 12:06:50 PDT
(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.
Comment 3 Eli Friedman 2021-09-07 13:43:37 PDT
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.
Comment 4 Sanjay Patel 2021-09-08 13:52:15 PDT
(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()) {
Comment 5 Eli Friedman 2021-09-08 14:12:18 PDT
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.
Comment 6 Eli Friedman 2021-09-08 14:14:53 PDT
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.
Comment 7 Sanjay Patel 2021-09-09 08:09:21 PDT
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
Comment 8 Sanjay Patel 2021-09-16 14:09:36 PDT
I'm confused about clang internals, but here's a proposal to add warnings:
https://reviews.llvm.org/D109925