[Mesa-dev] [PATCH] llvmpipe: fix fogcoord writing

[Jose Fonseca]

Looks good in principle. Just a few remarks inline on form.

----- Original Message -----
> From: Dave Airlie <airlied at redhat.com>
> 
> this fixes the fogcoord related piglit tests, like I fixed them in
> softpipe.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  src/gallium/auxiliary/draw/draw_llvm.c |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c
> b/src/gallium/auxiliary/draw/draw_llvm.c
> index e71c802..221aa17 100644
> --- a/src/gallium/auxiliary/draw/draw_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
> @@ -475,6 +475,8 @@ generate_vs(struct draw_llvm *llvm,
>     struct lp_type vs_type;
>     LLVMValueRef consts_ptr =
>     draw_jit_context_vs_constants(llvm->gallivm, context_ptr);
>     struct lp_build_sampler_soa *sampler = 0;
> +   unsigned attrib, chan;
> +   struct lp_build_context bld;
>  
>     memset(&vs_type, 0, sizeof vs_type);
>     vs_type.floating = TRUE; /* floating point values */
> @@ -504,11 +506,20 @@ generate_vs(struct draw_llvm *llvm,
>                       outputs,
>                       sampler,
>                       &llvm->draw->vs.vertex_shader->info);
> +   {
> +      struct tgsi_shader_info* info =
> &llvm->draw->vs.vertex_shader->info;

The `info` variable could be also declared on the top, and { } enclosing this block removed.

> +      lp_build_context_init(&bld, llvm->gallivm, vs_type);
> +      for (attrib = 0; attrib < info->num_outputs; ++attrib) {
> +         if (info->output_semantic_name[attrib] ==
> TGSI_SEMANTIC_FOG) {
> +            LLVMBuildStore(builder, bld.zero, outputs[attrib][1]);
> +            LLVMBuildStore(builder, bld.zero, outputs[attrib][2]);
> +            LLVMBuildStore(builder, bld.one, outputs[attrib][3]);
> +         }
> +      }
> +   }
>  
>     if (clamp_vertex_color) {
>        LLVMValueRef out;
> -      unsigned chan, attrib;
> -      struct lp_build_context bld;
>        struct tgsi_shader_info* info =
>        &llvm->draw->vs.vertex_shader->info;
>        lp_build_context_init(&bld, llvm->gallivm, vs_type);
>  
> @@ -522,6 +533,12 @@ generate_vs(struct draw_llvm *llvm,
>                    out = lp_build_clamp(&bld, out, bld.zero,
>                    bld.one);
>                    LLVMBuildStore(builder, out,
>                    outputs[attrib][chan]);
>                    break;
> +               case TGSI_SEMANTIC_FOG:
> +                  if (chan == 1 || chan == 2)
> +                     LLVMBuildStore(builder, bld.zero,
> outputs[attrib][chan]);
> +                  else if (chan == 3)
> +                     LLVMBuildStore(builder, bld.one,
> outputs[attrib][chan]);
> +                  break;

I think these lines just duplicates the above hunk.

Actually, an easier way to accomplish this is to fold the "if (clamp_vertex_color) { ... }" statement inside the TGSI_SEMANTIC_(B)COLOR case, and add the TGSI_SEMANTIC_FOG, which will be run inconditionally, as:


      for (attrib = 0; attrib < info->num_outputs; ++attrib) {
         for (chan = 0; chan < TGSI_NUM_CHANNELS; ++chan) {
            if (outputs[attrib][chan]) {
               switch (info->output_semantic_name[attrib]) {
               case TGSI_SEMANTIC_COLOR:
               case TGSI_SEMANTIC_BCOLOR:
                  if (clamp_vertex_color) {
                     LLVMValueRef out;
                     out = LLVMBuildLoad(builder, outputs[attrib][chan], "");
                     out = lp_build_clamp(&bld, out, bld.zero, bld.one);
                     LLVMBuildStore(builder, out, outputs[attrib][chan]);
                  }
                  break;
              case TGSI_SEMANTIC_FOG:
                 if (chan == 1 || chan == 2)
                    LLVMBuildStore(builder, bld.zero, outputs[attrib][chan]);
                 else if (chan == 3)
                    LLVMBuildStore(builder, bld.one, outputs[attrib][chan]);
                 break;
            }
         }
      }

Jose