-march=i686 causing lightmap corruption?

[Spirit]

I have no idea about engines or C.

I had a weird bug in recently, where apparently lightmaps were corrupted. Here is an example. Top is how it looked like, bottom is how it should look like.





Luckily I was able to bisect to the culprit: The compiler flag "-march=i686".

I am on a x86_64 Linux, nvidia binary drivers. If I recall correctly I added that flag to be able to compile a 32 bit binary. The engine is 32 bit (if you say it like that), at least I think it is.

How can this option cause this? Do I actually need it? Does it hint at some bug in the engine code?

I left it in the project for now, if you want to try it for yourself grab the source at https://github.com/SpiritQuaddicted/reQuiem and compile it with codeblocks.

[szo]

Does the following help?
http://sourceforge.net/p/quakespasm/patches/15/
http://sourceforge.net/p/quakespasm/code/908/

[Spirit]

Sadly not. But I found that there were similar blotches on mfxsp17 that this change fixed.

mfxsp17 before and after:



Some more screenshots of the issue, from jam1_scampie and jam1_ericw:

[Baker]

[szo]

It definitely implies that there is a bug somewhere, be that a bug
in C code or a bad use of a compiler flag.

I tried building it, the makefile was broken, fixed it. (can send to
you if you want.) Then realized that there is a codeblocks thing,
sigh.. (Why do I need codeblocks when there can be a simple makefile..
sigh^2...)

There are some "interesting" compiler flags in there. Things I noted:
* __linux__ defined among the CPPFLAGS, which is brain damage: your
compiler defines it already if it is targetting linux.
* -funsigned-char: why is it needed really???
* -fexpensive-optimizations: unless you are using gcc < 2.7 (or was
it <= 2.7??), it is already in effect.
* -fno-strength-reduce: why is this really needed?
* -funroll-loops: is it really worth the trouble??
* -O6 (in the makefiles): gcc highest optim.level is 3, -O6 is stupid
* -ffast-math: be careful with it.. (see the gcc man page.)

gl_brush.c is still broken: s/"contents ount"/count/ around line 1395

I didn't run into the broken lightmaps, but I only ran stock id1 maps
and only briefly. If you point me to the maps in question, maybe I can
look into it sometime.

[Spirit]

[szo]

Sent a small patch to your private email address.

I experimented with the cpu flags and -ffast-math: -march=pentium
(i586) is ok, but anything higher than that (-march=pentiumpro,
i686, pentium2. pentium3) in combination with -ffast-math reveals
the glitch you see. Seems like some floating point inaccuracy
somewhere. Howerver, either removing -ffast-math or switching to
SSE floating point math (i.e. -msse -mfpmath=sse in combination
with a proper -march, like -march=pentium3) "cures" the issue.
Didn't try to dig more.

[ericw]

Interesting..

Sounds like you nailed it down szo; I imagine -ffast-math is messing up that sensitive dot product in a way that lowers the precision a bit.
http://stackoverflow.com/questions/7420 ... ctually-do

On a related topic, the 'light' tool needs the same patch (adding the casts to increase the intermediate precision of that dot product), if you want to compile 'light' with sse. I sent it to Tyrann who applied it to tyrutils, so tyrutils from git is now safe to use as a 64-bit build.

Btw, I casted to 'long double' in the tyrutils patch, which is at least the size of double; in practice it's 128-bit on my laptop. Since the original code was being built for x87 (80-bit), i figured it's a bit safer than casting to double. Not sure if it will ever matter though.

[Spirit]

Thanks for the patches, I commited them.

I tried "-march=pentium4" and that worked.

Do I need either option anyways if I want to just build some binary for everyone to use? Should I have separate 32 and 64 binaries? It's all way over my head, I just want to release a proper build some day.

[szo]

[Spike]

if its just because your program truncates pointer precision due to it using the wrong data types, you can use -m32 and that'll work on x86, arm, or powerpc chips.
you can specify -m64, but that's generally default anyway (unless cross-compiling), and thus redundant.

I don't know what the actual difference is between the builds, but lightmap sizes are definitely dependant upon float precision. which sucks. you could try using the floor() libc function instead of casting to int perhaps? or add a tiny smidge extra first.