[Mesa-dev] Mesa (master): glsl: fix assorted MSVC warnings

[José Fonseca]

On Tue, 2010-11-16 at 11:55 -0800, Ian Romanick wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/15/2010 05:48 PM, Brian Paul wrote:
> 
> >     case ir_unop_b2f:
> >        assert(op[0]->type->base_type == GLSL_TYPE_BOOL);
> >        for (unsigned c = 0; c < op[0]->type->components(); c++) {
> > -	 data.f[c] = op[0]->value.b[c] ? 1.0 : 0.0;
> > +	 data.f[c] = op[0]->value.b[c] ? 1.0F : 0.0F;
> 
> Please don't do this.  This particular MSVC warning should just be
> disabled.  If this warning were generated for non-literals and for
> literals that actually did lose precision being stored to a float, it
> might have a chance at having some value.  Instead, it's just noise.
> 
> Individual warnings can be disabled with a pragma, and this one should
> probably be disabled in mesa/compiler.h:
> 
> #pragma warning(disable: 4244)
> 
> There may be a way to do it from the command line, but I don't know what
> it is.

It's -wd4244.

> The F suffixes on constants are also worthless, and they make the code
> ugly. 

I had the impression it was more than a warning, namely that the
compilers would use double precision intermediates instead of single
precision floats when constants don't have the 'f' suffix.

Gcc does it. Take for example:

         float foo(float x)
        {
        	return 1.0 / x + 5.0;
        }
        
        float foof(float x)
        {
        	return 1.0f / x + 5.0f;
        }
        
If you compile it on x64 with

    gcc -g0 -O3 -S -o - test.c

you'll get 
   
        	.file	"foo.c"
        	.text
        	.p2align 4,,15
        .globl foo
        	.type	foo, @function
        foo:
        .LFB0:
        	.cfi_startproc
        	unpcklps	%xmm0, %xmm0
        	cvtps2pd	%xmm0, %xmm1
        	movsd	.LC0(%rip), %xmm0
        	divsd	%xmm1, %xmm0
        	addsd	.LC1(%rip), %xmm0
        	unpcklpd	%xmm0, %xmm0
        	cvtpd2ps	%xmm0, %xmm0
        	ret
        	.cfi_endproc
        .LFE0:
        	.size	foo, .-foo
        	.p2align 4,,15
        .globl foof
        	.type	foof, @function
        foof:
        .LFB1:
        	.cfi_startproc
        	movaps	%xmm0, %xmm1
        	movss	.LC2(%rip), %xmm0
        	divss	%xmm1, %xmm0
        	addss	.LC3(%rip), %xmm0
        	ret
        	.cfi_endproc
        .LFE1:
        	.size	foof, .-foof
        	.section	.rodata.cst8,"aM", at progbits,8
        	.align 8
        .LC0:
        	.long	0
        	.long	1072693248
        	.align 8
        .LC1:
        	.long	0
        	.long	1075052544
        	.section	.rodata.cst4,"aM", at progbits,4
        	.align 4
        .LC2:
        	.long	1065353216
        	.align 4
        .LC3:
        	.long	1084227584
        	.ident	"GCC: (Debian 4.4.5-6) 4.4.5"
        	.section	.note.GNU-stack,"", at progbits

And as you can see, one function uses double precision, and the other
uses floating point.

Code quality is much better in the latter.

> Expecting that they will be added everywhere when no other
> compiler generates this warning is a losing battle.

I really think this is a battle everybody should fight. Perhaps the 

   condition ? 1.0 : 0.0

is something that a compiler should eliminate, but "single precision
expressions should use 'f' suffix on constants" seems to be a good rule
of thumb to follow.

Jose