The C backend cannot emit NAN's and other non-number values (such as infinity) as global variable initializers. This is tested by: test/Regression/CBackend/2003-10-12-NANGlobalInits.ll
Here is a proposed patch for NaN and Inf cases: Index: Writer.cpp =================================================================== RCS file: /var/cvs/llvm/llvm/lib/CWriter/Writer.cpp,v retrieving revision 1.143 diff -a -u -r1.143 Writer.cpp --- Writer.cpp 11 Nov 2003 22:41:32 -0000 1.143 +++ Writer.cpp 13 Nov 2003 03:14:05 -0000 @@ -457,14 +457,25 @@ Out << "(*(" << (FPC->getType() == Type::FloatTy ? "float" : "double") << "*)&FPConstant" << I->second << ")"; } else { + std::string Num; #if HAVE_PRINTF_A // Print out the constant as a floating point number. char Buffer[100]; + sprintf(Buffer, "%a", FPC->getValue()); - Out << Buffer << " /*" << FPC->getValue() << "*/ "; + Num = Buffer; #else - Out << ftostr(FPC->getValue()); + Num = ftostr(FPC->getValue()); #endif + + if (Num == "nan") + // The value is NaN + Out << "LLVM_NAN /*nan*/ "; + else if (Num == "inf") + // The value is Inf + Out << "LLVM_INF /*inf*/ "; + else + Out << Num; } break; } @@ -580,6 +591,15 @@ // If we aren't being compiled with GCC, just drop these attributes. Out << "#ifndef __GNUC__ /* Can only support \"linkonce\" vars with GCC */\n" << "#define __attribute__(X)\n" + << "#endif\n\n"; + + // Define NaN and Inf as GCC builtins if using GCC, as 0 otherwise. + Out << "#ifdef __GNUC__\n" + << "#define LLVM_NAN __builtin_nan(\"\")\n" + << "#define LLVM_INF __builtin_inf()\n" + << "#else\n" + << "#define LLVM_NAN 0\n" + << "#define LLVM_INF 0\n" << "#endif\n"; }
Looks like a great first start. However, there are multiple different kinds of nan's (hence the argument to the builtin) and there are both + and - infinities. Can we support these as well? :) -Chris
Sure. :-) -Bill
This bug was present in 1.0, but hopefully Bill will fix it for 1.1 :) -Chris
When you ask about the multiple different types of builtins, are you refering to the nans and huge_val? If so, I'm not sure when would be the best situation to use these. (huge_val doesn't produce a warning like inf does and nans produces a "signaling" NaN, which I have no clue what that is...)
Suggested patch: cvs server: Diffing . Index: Writer.cpp =================================================================== RCS file: /var/cvs/llvm/llvm/lib/CWriter/Writer.cpp,v retrieving revision 1.144 diff -a -u -r1.144 Writer.cpp --- Writer.cpp 16 Nov 2003 22:06:14 -0000 1.144 +++ Writer.cpp 18 Nov 2003 23:24:43 -0000 @@ -451,20 +451,45 @@ case Type::DoubleTyID: { ConstantFP *FPC = cast<ConstantFP>(CPV); std::map<const ConstantFP*, unsigned>::iterator I = FPConstantMap.find(FPC); + if (I != FPConstantMap.end()) { // Because of FP precision problems we must load from a stack allocated // value that holds the value in hex. Out << "(*(" << (FPC->getType() == Type::FloatTy ? "float" : "double") << "*)&FPConstant" << I->second << ")"; } else { + std::string Num; #if HAVE_PRINTF_A // Print out the constant as a floating point number. char Buffer[100]; + sprintf(Buffer, "%a", FPC->getValue()); - Out << Buffer << " /*" << FPC->getValue() << "*/ "; + Num = Buffer; #else - Out << ftostr(FPC->getValue()); + Num = ftostr(FPC->getValue()); #endif + + if (Num == "nan") { + // The value is NaN + if (FPC->getType() == Type::FloatTy) + Out << "LLVM_NANF /*nan*/ "; + else + Out << "LLVM_NAN /*nan*/ "; + } else if (Num == "inf") { + // The value is +Inf + if (FPC->getType() == Type::FloatTy) + Out << "LLVM_INFF /*inf*/ "; + else + Out << "LLVM_INF /*inf*/ "; + } else if (Num == "-inf") { + // The value is -Inf + if (FPC->getType() == Type::FloatTy) + Out << "-LLVM_INFF /*inf*/ "; + else + Out << "-LLVM_INF /*inf*/ "; + } else { + Out << Num; + } } break; } @@ -580,6 +605,19 @@ // If we aren't being compiled with GCC, just drop these attributes. Out << "#ifndef __GNUC__ /* Can only support \"linkonce\" vars with GCC */\n" << "#define __attribute__(X)\n" + << "#endif\n\n"; + + // Define NaN and Inf as GCC builtins if using GCC, as 0 otherwise. + Out << "#ifdef __GNUC__\n" + << "#define LLVM_NAN __builtin_nan(\"\") /* Double */\n" + << "#define LLVM_NANF __builtin_nanf(\"\") /* Float */\n" + << "#define LLVM_INF __builtin_inf() /* Double */\n" + << "#define LLVM_INFF __builtin_inff() /* Float */\n" + << "#else\n" + << "#define LLVM_NAN ((double)0.0) /* Double */\n" + << "#define LLVM_NANF 0.0F /* Float */\n" + << "#define LLVM_INF ((double)0.0) /* Double */\n" + << "#define LLVM_INFF 0.0F /* Float */\n" << "#endif\n"; }
The patch is definitely looking better... > When you ask about the multiple different types of builtins, are you refering to > the nans and huge_val? If so, I'm not sure when would be the best situation to > use these. (huge_val doesn't produce a warning like inf does and nans produces a > "signaling" NaN, which I have no clue what that is...) The basic idea is that the C backend should be able to translate ANY IEEE bitpattern exactly to the C compiler. Some bit-patterns represent numbers which can be printed exactly as FP numbers, such as 1.0. Others are normal numbers, but cannot be printed exactly in decimal. We handle these with C99 "hex FP" extensions. The class if bit patterns this bug is talking about is those classified as NANs, SNANs, and INFs. There is a whole class of these values (not just 3 for float and double) and we want to translate ALL of them bit-exactly to the C compiler. The argument to builtin_nan specifies _which_ NAN to generate. If my memory is correct, NAN's all have an exponent of 128 or something, but the mantissa can be anything. Correctly telling the C compiler about this requires using the string argument to builtin_nan. Floating point is a wierd disturbing thing. I'd recommend skimming the article "what every computer scientist ought to know about floating point". -Chris
I don't think that Bill is looking at this bug anymore. If you are, please feel free to take back ownership of the bug Bill. -Chris
Sorry. I dropped the ball on this. With moving and not having internet connections, etc. I hadn't had the time to look into it further. I'll still be working on it a bit but won't revert the ownership to me until I get something working on my end. Bill
Changing all of these bugs who do not have people looking at them to be assigned to "unassignedbugs", indicating that if someone is feeling ambitious, they can take ownership of the bug. If I stole your bug, and you still want it, feel free to take ownership back. -Chris
Here's another shot at this. It allows for bit-precise NaNs by feeding the actual hex value of the FP into the __builtin_nan(...) built-in. One thing. What should be the default for non-GNUC compilers? Right now, it's just 0.0, which can't be correct. Index: Writer.cpp =================================================================== RCS file: /var/cvs/llvm/llvm/lib/Target/CBackend/Writer.cpp,v retrieving revision 1.188 diff -a -u -r1.188 Writer.cpp --- Writer.cpp 12 Jul 2004 23:37:18 -0000 1.188 +++ Writer.cpp 13 Jul 2004 04:25:17 -0000 @@ -556,14 +556,37 @@ Out << "(*(" << (FPC->getType() == Type::FloatTy ? "float" : "double") << "*)&FPConstant" << I->second << ")"; } else { + std::string Num; #if HAVE_PRINTF_A // Print out the constant as a floating point number. char Buffer[100]; sprintf(Buffer, "%a", FPC->getValue()); - Out << Buffer << " /*" << FPC->getValue() << "*/ "; + Num = Buffer; #else - Out << ftostr(FPC->getValue()); + Num = ftostr(FPC->getValue()); #endif + + if (Num == "nan") { + // The value is NaN + if (FPC->getType() == Type::FloatTy) + Out << "LLVM_NANF(\"" << Num << "\") /*nan*/ "; + else + Out << "LLVM_NAN(\"" << Num << "\") /*nan*/ "; + } else if (Num == "inf") { + // The value is +Inf + if (FPC->getType() == Type::FloatTy) + Out << "LLVM_INFF /*inf*/ "; + else + Out << "LLVM_INF /*inf*/ "; + } else if (Num == "-inf") { + // The value is -Inf + if (FPC->getType() == Type::FloatTy) + Out << "-LLVM_INFF /*inf*/ "; + else + Out << "-LLVM_INF /*inf*/ "; + } else { + Out << Num; + } } break; } @@ -699,6 +722,49 @@ << "#else\n" << "#define __ATTRIBUTE_WEAK__\n" << "#endif\n\n"; + + // Define NaN and Inf as GCC builtins if using GCC, as 0 otherwise + // From the GCC documentation: + // + // double __builtin_nan (const char *str) + // + // This is an implementation of the ISO C99 function nan. + // + // Since ISO C99 defines this function in terms of strtod, which we do + // not implement, a description of the parsing is in order. The string is + // parsed as by strtol; that is, the base is recognized by leading 0 or + // 0x prefixes. The number parsed is placed in the significand such that + // the least significant bit of the number is at the least significant + // bit of the significand. The number is truncated to fit the significand + // field provided. The significand is forced to be a quiet NaN. + // + // This function, if given a string literal, is evaluated early enough + // that it is considered a compile-time constant. + // + // float __builtin_nanf (const char *str) + // + // Similar to __builtin_nan, except the return type is float. + // + // double __builtin_inf (void) + // + // Similar to __builtin_huge_val, except a warning is generated if the + // target floating-point format does not support infinities. This + // function is suitable for implementing the ISO C99 macro INFINITY. + // + // float __builtin_inff (void) + // + // Similar to __builtin_inf, except the return type is float. + Out << "#ifdef __GNUC__\n" + << "#define LLVM_NAN(NanStr) __builtin_nan(NanStr) /* Double */\n" + << "#define LLVM_NANF(NanStr) __builtin_nan(NanStr) /* Float */\n" + << "#define LLVM_INF __builtin_inf() /* Double */\n" + << "#define LLVM_INFF __builtin_inff() /* Float */\n" + << "#else\n" + << "#define LLVM_NAN(NanStr) ((double)0.0) /* Double */\n" + << "#define LLVM_NANF(NanStr) 0.0F /* Float */\n" + << "#define LLVM_INF ((double)0.0) /* Double */\n" + << "#define LLVM_INFF 0.0F /* Float */\n" + << "#endif\n"; } bool CWriter::doInitialization(Module &M) {
We can try out your patch, but don't close the bug until it's fixed in CVS :) -Chris
Sorry. I jumped the gun on that. I didn't realize what the "WORKSFORME" option did. :-) -bw
The patch looks basically good, however I don't like the text comparisons like this: 'if (Num == "nan")'. Instead could you use the IsNan function from Support/IsNan.h, and perhaps come up with a new function for infinity checking? -Chris
Created attachment 154 [details] Configuration regen for "isinf" check and new NaN and Inf handling This is the "cvs diff" of my CVS tree after running AutoRegen.sh. I also need to add the IsInf.cpp file...That's coming up.
Created attachment 155 [details] "isinf" support Here's the implementation of the "isinf" support.
Created attachment 156 [details] Configuration regen for "isinf" check and new NaN and Inf handling (Redux) The previous patch doesn't compile...*sigh*
K, two things * Please resend just the isinf autoconf changes (without regenerated files) as one change. I am not one that can do this, but John or Brian can approve and apply that. * Once that is in, the writer.cpp change can go in. Please reattach as a separate file, and make sure that tabs don't sneak in :) Thanks Bill, I'm sorry I dropped the ball on this one! -Chris
Created attachment 157 [details] Patches to the (non-generated) configuration files to support the "IsInf" function. Once applied, you should regenerate the configure code like normal.
Created attachment 158 [details] Code patch to add support for proper NaN and Inf handling Adds header information to the MathExtras.h file and the code to the CodeGen/CBackend/Writer.cpp file.
Created attachment 159 [details] IsInf functions Supplies the "IsInf" functions. Resides in the llvm/lib/Support directory.
Created attachment 160 [details] Patches to the (non-generated) configuration files to support the "IsInf" function This doesn't have the "MathExtras.h" patc included, which is already included in #158.
I fear there's something yet wrong with this patch. When I try to compile the C-Backend generated code for 2003-10-12-NANGlobalInits.ll with this patch applied, I get an error from gcc: 101 brg@aluminum:~/llvm/test/Regression/CodeGen/CBackend% llvm-as < 2003-10-12- NANGlobalInits.ll | llc -f -march=c -o=- > foo.c [...] 105 brg@aluminum:~/llvm/test/Regression/CodeGen/CBackend% gcc -S foo.c foo.c:76: error: initializer element is not constant foo.c:76: error: (near initialization for `NAN.field0')
114 brg@aluminum:~/llvm/test/Regression/CodeGen/CBackend% cat foo.c struct l_unnamed0 { float field0; }; struct l_unnamed0 NAN = { __builtin_nan("nan") }; 115 brg@aluminum:~/llvm/test/Regression/CodeGen/CBackend% gcc -c foo.c foo.c:2: error: initializer element is not constant foo.c:2: error: (near initialization for `NAN.field0') It appears that the behavior of __builtin_nan doesn't exactly match its description in the gcc manual! (the part about "considered a compile-time constant", that is...)
I think the problem is in the use of "nan" as the initializer for __builtin_nan(). If you use "" or a string with a hex value that's equivalent to NaN, then it'll be okay: #include <stdio.h> #include <math.h> struct { double d; } S = { __builtin_nan("0x1.2f6a7ef9db22dp+5") }; int main() { printf("S.d == %f\n", S.d); } works when compiling with g++. __builtin_nan("") works with gcc.
Ah... I see the problem now: I was testing on a non-HAVE_PRINTF_A platform (Mac OS X). I'll check the patches in and try it on a Linux machine.
Actually, I was mistaken - Mac OS X does HAVE_PRINTF_A, but For a, A, e, E, f, F, g, and G conversions, positive and negative infinity are represented as inf and -inf respectively when using the lowercase conversion character, and INF and -INF respectively when using the uppercase conversion character. Similarly, NaN is represented as nan when using the lowercase conversion, and NAN when using the uppercase conversion. So you'll never get hex values for a NaN out of it. Crud.
*@&%~! Craptastic...Um...what to do then? Is there a platform-indenpendent way of getting the hex value of a FP number?
Well we do currently assume that they are using IEEE 32/64 bit numbers. You could pull the bits out yourself...
union { double d; uint64_t u; } x; x.d = <doubleval>; sprintf (<buffer>, "0x%llx", x.u);
Created attachment 162 [details] Fix for converting FP value to Hex Thanks Brian for the suggestion.
I thought that the __builtin_nan only took the nan mantissa value, not the whole FP #... -Chris
reassigning so I stop getting 2x mails :)
If __builtin_nan() is really supposed to be like C99 nan(), then it's implementation-defined...
Then why bother sending a bit pattern at all? -Chris
Good question. One thing is that __builtin_nan() seems to force it to be a "quiet" NaN (at least according to the documentation). To do a "signalling" NaN, you need to use __builtin_nans(). Since you're guaranteeing a format for FP#s, we could grab the mantissa, determine what type of NaN it is, then issue that builtin (either with "" or with a hex string for that platform-dependent behaviour people want).
Sounds groovy to me. Getting QNAN vs SNAN right is important. Distinguishing between various SNANs is much less so :) -Chris
<burns>Excellent!</burns> I'll see about doing this tonight. :-) -bw
Created attachment 163 [details] Fix to get the mantissa to determine if this is a quiet or signalling NaN.
Comment on attachment 163 [details] Fix to get the mantissa to determine if this is a quiet or signalling NaN. This should be it. :-) It now checks that the FP # starts with 0x7FF8 or 0x7FF4 for quiet and signalling NaNs (resp.). I also added another value to the 2003-10-12-NANGlobal.ll test.
FYI: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040719/016493.html -Chris
Hi Chris, Did you see the newest patch I submitted to this bug? It should fix the outputting of a "nan" for the LLVM_NAN tag...and also implement the signalling and quiet NaNs. -bw
Brian, Did you get a chance to try out this fix? Fix to get the mantissa to determine if this is a quiet or signalling NaN. 2004-07-22 00:01 -bw
So, I've applied the patches, and the test case seems to work now. I'm wondering if anyone else has further tests to suggest? If not, then I move that we close this bug.
If it fixed the problem and causes no regressions, I say close it! Thanks Bill and Brian! -Chris
Looks like we're done here.
Thanks to Brian who caught a completely brain-dead mistake that I made. -bw