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",@progbits,8
.align 8
.LC0:
.long 0
.long 1072693248
.align 8
.LC1:
.long 0
.long 1075052544
.section .rodata.cst4,"aM",@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,"",@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
