[llvmgcc] C front-end miscompiles float constants on big-endian 32-bit target

[Quuxplusone]

Bugzilla Link	PR164
Status	RESOLVED FIXED
Importance	P normal
Reported by	John T. Criswell (jtcriswel@gmail.com)
Reported on	2003-12-05 16:40:32 -0800
Last modified on	2010-02-22 12:42:49 -0800
Version	1.0
Hardware	Sun Solaris
CC	clattner@nondot.org, gaeke+bugs@uiuc.edu, llvm-bugs@lists.llvm.org, mkahl@apple.com
Fixed by commit(s)
Attachments	minus.c (68 bytes, text/plain) minus.o (196 bytes, application/octet-stream) llvm-expand-patch (2188 bytes, text/plain) llvm-expand-patch (2560 bytes, text/plain)
Blocks
Blocked by
See also

On Sparc/Solaris, the funcresolv pass is changing a float constant passed as an
argument to a function to a different value (in this case, 0).  This pass is run
by gccld by default and is causing several Sparc test causes to fail.

[Quuxplusone]

The funcresolv pass appears to be in lib/Transforms/IPO, which I assume is an
interprocedureal optimization.

[Quuxplusone]

Testcase please!

[Quuxplusone]

I consider this PR to be blocking the 1.1 release, so please send me a testcase
so we can resolve it.

Thanks,

-Chris

[Quuxplusone]

Sorry for the delay.

Testcase is llvm/test/Programs/SingleSource/UnitTests/2002-05-02-ArgumentTest.c
If you run this testcase through the optimizer with the funcresolv pass, you
should see the first float parameter to TestFun() changed to zero.

[Quuxplusone]

As it turns out, this problem was because the Sparc front-end was accidentally
configured for V8, not V9 operation.  Still, the C front-end is miscompiling
floating point constants in this configuration.

Brian, isn't the PPC big-endian, 32-bit?  If so, does
llvm/test/Programs/SingleSource/UnitTests/2002-05-02-ArgumentTest.c work on it?

-Chris

[Quuxplusone]

No wait. I'm not sure what is going on here.  I built a C front-end for 32-bit
sparc V8, and it is producing the correct floating point constant.  I'm not sure
what happened John, but this doesn't seem to be a problem.  Thanks for updating
the docs though!

-Chris

[Quuxplusone]

Attached minus.c (68 bytes, text/plain): source file "minus.c"

[Quuxplusone]

Attached minus.o (196 bytes, application/octet-stream): result of compiling "minus.c"

[Quuxplusone]

The above-mentioned test does indeed fail on MacOSX.  Both the float and double
arguments are
incorrect.  From the disassembled IR:

        call void (short, float, sbyte, long, int, double)* %testfunc(short 12, float 0x200000000045D25C,
sbyte 120, long 123456677890, int -10, double 0xCAFA80000045D25C)

In other experiments, I've found that ALL floats and doubles end in "0045D25C"!
I can't decode IEEE
floating-point in my head (any more :-), but that's surely wrong.  Even 0.0
doesn't come out all-bits-
zero.

It is even easy to create invalid bytecode files!  I've attached a very short
source file, "minus.c".  Here is
what happens:

        [hatter:~/Work/llvm/test] % llvm-gcc -c minus.c

        [hatter:~/Work/llvm/test] % opt -print minus.o
        /Volumes/Users/mkahl/Work/llvm/llvm/lib/Bytecode/Reader/Reader.cpp:95: failed assertion `(!
isa<Constant>(Val) || !cast<Constant>(Val)->isNullValue()) ||
!hasImplicitNull(type,
hasExplicitPrimitiveZeros) && "Cannot read null values from bytecode!"'
        Abort trap

I've also attached "minus.o", in case that helps.

[Quuxplusone]

It is possible that the assertion failure you are seeing is a separate issue
related to recent bytecode
reader/writer changes. I was seeing lots of those on SPARC recently.

I am also seeing the bogus hex digits at the end of floating point constants on
PPC, but I get
different ones, so I suspect it results from some uninitialized state somewhere
in the C front-end.

[Quuxplusone]

A long, long time ago in a CS building across the street, I actually fixed this
bug for SparcV9, but the fix probably isn't correct for all the different
combinations of endianess/integer bit sizes.

If I had to guess, I'd say this is a bug in the CFE, in gcc/llvm-expand.c, in
the llvm_expand_constant_expr() function.  The code to handle a REAL_CST is
probably incorrect.

Brian (or anybody), if you can get me a PPC MacOS X machine that I can compile
on, I can go ahead and try to fix this.

[Quuxplusone]

I may have time to look into this tonight. It quite possibly is a CFE issue.

Sorry, I have to run, I'm at the airport :)

-Chris

[Quuxplusone]

Wow, thanks John, that's basically what I was going to propose!  :)

One question, should this:
              Values = (char*)RealArr;
be this:
              Values = (char*)RealArr+4;

?  I assume that this will be found via testing.

Also, can you fix the "apperas" type-o while you're in there? :)

-Chris

[Quuxplusone]

I believe the offset is correct.  The RealArr+4 for 64 bit is because on
SparcV9, those longs are actually 64 bits (instead of 32 bits).  The addition of
4 bytes skips over to the lower 32 bits of each longword in the returned
structure.

For a host machine with 32 bit longs and 32 bit FP numbers, no offset should be
needed.

Even if this fixes MacOS X, the code as a whole is still not entirely correct.
For example, if GCC had been compiled as a SparcV8 binary but was compiling for
SparcV9, the code, as is, won't work properly.  Same thing for LP code on little
endian machines.  Same thing for cross compilers.

At some point, we need to go in and figure out how to do this bit of code
correctly, which is tedious.

[Quuxplusone]

> Even if this fixes MacOS X, the code as a whole is still not entirely correct.
> For example, if GCC had been compiled as a SparcV8 binary but was compiling
for
> SparcV9, the code, as is, won't work properly.  Same thing for LP code on
little
> endian machines.  Same thing for cross compilers.

I _totally_ agree.  It is quite disgusting, and I'm sure there is a better way.
 If you find a nice way to eviscerate it, please do.

-Chris

[Quuxplusone]

Attached llvm-expand-patch (2188 bytes, text/plain): Proposed fix to gcc-3.4/gcc/llvm-expand.c

[Quuxplusone]

Attached llvm-expand-patch (2560 bytes, text/plain): Proposed fix: gcc-3.4/gcc/llvm-expand.c

[Quuxplusone]

Assuming that your patch works on X86 and Sparc, go ahead and check it in John.
 It can't make things any worse on PPC, but hopefully it will make things better.

-Chris

[Quuxplusone]

For the original proposed fix, I seem to get correct output for

  extern double x(double); double y() { return x(0.1234567); }

the hex output is "double 0x3FBF9ADBB8F8DA72", which is precisely the same
bit pattern that native gcc outputs (but gcc outputs 2 words.) This is fine.

When I try the same program under the substitution s/double/float/g, I get
0x3DFCD6DE from native gcc, and 0x3FBF9ADBC0000000 from llvm-gcc. I'm nervous
about the fact that these are not also identical, and that llvm-gcc is
outputting a double.

I haven't tried the 2nd proposed fix yet.

[Quuxplusone]

This is "expected behavior".  If you assemble and disassemble the testcase, it
should give you the right FP value with floats.

-Chris

[Quuxplusone]

2nd proposed fix gives identical behavior. Also seems to work w/ 0.0.
I say ship it.

[Quuxplusone]

The patch works for me.

[Quuxplusone]

John fixed this bug, patch here:
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040126/011010.html

-Chris