[virglrenderer-devel] [PATCH] vrend_shader: add arb_gpu_shader_fp64 support (v2)

[Dave Airlie]

>> +
>
> Ideally, we would get rid of fp64_dst and fp64_src altogether.  For
> example, currently this code converts:
>
> D2F TEMP[0].x, CONST[0].xyxy
>
> to
>
> fp64_src[0].x = packDouble2x32(uvec2((fsconst0[0].xyxy).xyxy))
> temp0[0].x = float(fp64_src[0])
>
> In theory, it could just be
>
> temp[0].x = float(packDouble2x32(uvec2((fsconst0[0].xyxy)))
>
> For operations that involve a destination, we'll need multiple
> conversions.  For example,
>
> DMAX TEMP[0].xy, CONST[0].xyxy, CONST[1].xyxy
>
> is converted to
>
> fp64_src[0].x = packDouble2x32(uvec2((fsconst0[0].xyxy).xyxy));
> fp64_src[1].x = packDouble2x32(uvec2((fsconst0[1].xyxy).xyxy));
> fp64_dst[0].x = double((max(fp64_src[0], fp64_src[1])));
> temp0[0].xy = uintBitsToFloat(unpackDouble2x32(fp64_dst[0].x));
>
> It could be this longer statement:
>
> temp0[0].xy = uintBitsToFloat(unpackDouble2x32(double(max(packDouble2x32(uvec2((fsconst0[0].xyxy).xyxy))),packDouble2x32(uvec2((fsconst0[1].xyxy).xyxy))))))

My question is which one of those is "cleaner" I suppose.

They are both ugly, the first one takes up more lines but I think we
can work out what it is doing a lot easier. I tried to keep the packs
out of the main flow
for that reason as it makes it easier to spot bugs inside the pack/unpacks.

>From the pov of the consuming compiler I doubt either of these will
make a dent in what it produces.

Though also fp64 is such a niche corner case, I doubt we'll see
anything outside of test suites using it, so I'm not too sure how much
we want to trade off
cleanliness of produced code vs maintainability of the shader translator.

>
> We could also remove the "strcpy(writemask, ".x");" on compares with
> this.  It would be nice to introduce a strong typing system (temporary
> registers --> VEC4s, const registers --> UVEC4, immediate --> varies
> but still typed).  That way conversions would be easy.  Still, most
> Piglit tests (./piglit run tests/all -t arb_gpu_shader_fp64) seem to
> pass and something is better than nothing, so this patch is:
>
> Reviewed-by: Gurchetan Singh <gurchetansingh at chromium.org>
> Tested-by: Gurchetan Singh <gurchetansingh at chromium.org>

Thanks,
Dave.