Skip to content
Snippets Groups Projects

vkd3d-shader/tpf: Replace sm4 register structs with vkd3d-shader register structs, part 4.

Merged Francisco Casas requested to merge fcasas/vkd3d:use_vkd3d_reg_5_part4 into master
2 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Giovanni Mascellani approved this merge request

    approved this merge request

    • +        default:
      +            FIXME("Unhandled register modifier %#x.\n", src->modifiers);
      +            mod_token = VKD3D_SM4_REGISTER_MODIFIER_NONE;
      +            break;

      That should cause compilation to fail, right? (And probably with something like an internal compiler error, because this is something vkd3d-shader itself would trigger.)

      -static uint32_t sm4_src_register_order(const struct sm4_src_register *src)
      +static uint32_t vkd3d_shader_src_param_order(const struct vkd3d_shader_src_param *src)

      The naming here seems a bit unfortunate. As far as I can tell, this function determines the TPF source token count, which isn't a property of the vsir source parameter as such. I.e., I'd expect this to be called something along the lines of "sm4_token_count_from_src_register()", although you could possibly make a case for something like "vsir_src_register_get_tpf_token_count()". (vkd3d_shader_dst_param_order() has that issue as well.)

    • Author Developer

      That should cause compilation to fail, right? (And probably with something like an internal compiler error, because this is something vkd3d-shader itself would trigger.)

      Right. We don't want incorrect shaders to compile. I will put

      ERR("Unhandled register modifier %#x.\n", src->modifiers);
      vkd3d_unreachable();

      then.

      The naming here seems a bit unfortunate. As far as I can tell, this function determines the TPF source token count, which isn't a property of the vsir source parameter as such. I.e., I'd expect this to be called something along the lines of "sm4_token_count_from_src_register()", although you could possibly make a case for something like "vsir_src_register_get_tpf_token_count()". (vkd3d_shader_dst_param_order() has that issue as well.)

      "sm4_token_count_from_src_register()" sounds good to me, since other static functions in tpf.c start with "sm4_" too. I also renamed vkd3d_shader_dst_param_order().

    • Please register or sign in to reply
  • Francisco Casas added 3 commits

    added 3 commits

    • f36b35fa - vkd3d-shader/tpf: Make sm4_src_register.mod a vkd3d_shader_src_modifier enum.
    • ab00716c - vkd3d-shader/tpf: Remove sm4_src_register.swizzle_type.
    • ed91fb05 - vkd3d-shader/tpf: Replace sm4_src_register with vkd3d_shader_src_param.

    Compare with previous version

    • That should cause compilation to fail, right? (And probably with something like an internal compiler error, because this is something vkd3d-shader itself would trigger.)

      Right. We don't want incorrect shaders to compile. I will put

      ERR("Unhandled register modifier %#x.\n", src->modifiers);
      vkd3d_unreachable();

      then.

      I'll approve it, but I don't think that's terribly robust. Much like assert() and abort(), this escalates what would by definition be a bug in our code from causing compilation failure to causing program termination.

      @@ -4001,9 +3995,9 @@ static void write_sm4_instruction(const struct tpf_writer *tpf, const struct sm4
       
           size += instr->modifier_count;
           for (i = 0; i < instr->dst_count; ++i)
      -        size += vkd3d_shader_dst_param_order(&instr->dsts[i]);
      +        size += sm4_token_count_from_dst_register(&instr->dsts[i]);
           for (i = 0; i < instr->src_count; ++i)
      -        size += sm4_src_register_order(&instr->srcs[i]);
      +        size += sm4_token_count_from_src_register(&instr->srcs[i]);
           size += instr->idx_count;
           if (instr->byte_stride)
               ++size;

      As an aside, note that we don't necessarily need to calculate the size in advance here; we could do something like this:

          token_position = put_u32(buffer, 0);
          ...
          size = (buffer->size - token_position) / sizeof(uint32_t);
          token |= (size << VKD3D_SM4_INSTRUCTION_LENGTH_SHIFT);
          set_u32(buffer, token_position, token);

      Might be a nice follow-up to this series.

    • Author Developer

      I see. Nice observation.

    • Please register or sign in to reply
  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 11 commits

    added 11 commits

    • ed91fb05...679203f7 - 8 commits from branch wine:master
    • 32f03468 - vkd3d-shader/tpf: Make sm4_src_register.mod a vkd3d_shader_src_modifier enum.
    • 13f62e60 - vkd3d-shader/tpf: Remove sm4_src_register.swizzle_type.
    • c9277265 - vkd3d-shader/tpf: Replace sm4_src_register with vkd3d_shader_src_param.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading