Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Add determinant() function.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:det into master

I wanted to use ._xy element access to make it shorter, but that attempt was crushed by the reality - such indexing apparently compiles but does not produce correct element access loads. I'm going to update once this is fixed.

Merge request reports

Checking pipeline status.

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Mar 8, 2024 10:44pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Elizabeth Figura
  • Nikolay Sivov added 1 commit

    added 1 commit

    • 7895d067 - vkd3d-shader/hlsl: Add determinant() function.

    Compare with previous version

  • Nikolay Sivov resolved all threads

    resolved all threads

  • Nikolay Sivov added 1 commit

    added 1 commit

    • 14cac1b4 - vkd3d-shader/hlsl: Add determinant() function.

    Compare with previous version

  • Author Developer

    Turned out explicit conversion is already happening when function call is added, so it's again removed.

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Nice!

  • Francisco Casas approved this merge request

    approved this merge request

  • Giovanni Mascellani
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit 14cac1b4
  • 2883 if (type->class != HLSL_CLASS_SCALAR && type->class != HLSL_CLASS_MATRIX)
    2884 {
    2885 hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, "Invalid argument type.");
    2886 return false;
    2887 }
    2888
    2889 dim = min(type->dimx, type->dimy);
    2890 if (dim == 1)
    2891 {
    2892 if (!(arg = intrinsic_float_convert_arg(ctx, params, arg, loc)))
    2893 return false;
    2894 return hlsl_add_load_component(ctx, params->instrs, arg, 0, loc);
    2895 }
    2896
    2897 if (!(func = hlsl_compile_internal_function(ctx, "determinant", bodies[dim])))
    2898 return false;
    • Comment on lines +2897 to +2898

      I would guess that intrinsic_float_convert_arg() is needed here too. Or do you have evidence that the determinant of a matrix of halves is float rather than half?

    • Author Developer

      How do you suggest I verify that?

    • The trick I know takes advantage of function overloading:

      float test(float x) { return 1.0; }
      float test(half x) { return 2.0; }
      float main() { return test(determinant(x)); }
    • Author Developer

      Right, for half arguments it returns half result, judging by test() overload it picks. So that probably means I actually do need a template to produce different return type. It's also annoying if internal variables can't be kept as floats.

    • Yeah, it's quite unfortunate. Though notice that in !310 (comment 43922) a kind of solution was suggested for avoiding writing the type too many times. We might even introduce some helper doing sprintf(), compilation and memory handling all at once.

    • Author Developer

      I'd rather we errored-out on half's for now.

    • Author Developer

      Or maybe add a template for arguments/return type, and leave internal variables as floats at all times, if that works for casts.

    • Not sure of what the others prefer, but I'd push for the proper solution instead. It's not that hard, and with a little tooling it should be simply reusable in other future cases.

    • I don't think I care enough on this question. It seems easy, but halves are indeed rarely used. We'll need to fix lit() though; I'll send a patch for that one.

      If we really care about not specifying the printf argument multiple times, we can use something like [1] and [2]. I do worry though, since this means we're not using ANSI stdio anymore, and maybe that'll break something subtle.

      [1] zfigura/vkd3d@1277bbe8 [2] zfigura/vkd3d@93ca3d25

    • Author Developer

      Making sure I get this right. The proper solution would be to generate float vs half functions? For positional arguments we'll have a single "%1$s" format with a single typename argument?

    • Making sure I get this right. The proper solution would be to generate float vs half functions? For positional arguments we'll have a single "%1$s" format with a single typename argument?

      Yes, that's what I would try. Zeb has some concerns about breaking something with her patch that plugs in _vsprintf_p(), though. I'm willing to take the risk, though, but she definitely knows better about these kind of issues than I do. We're probably waiting for 1.9 to be released anyway.

    • Please register or sign in to reply
    • Resolved by Nikolay Sivov

      Also, could you please add a test like this one:

      [pixel shader]
      float3x4 m;
      
      float4 main() : sv_target
      {
          return determinant(m);
      }
      
      [test]
      uniform  0 float4  1.0  2.0  3.0 0.0
      uniform  4 float4  5.0 -6.0  7.0 0.0
      uniform  8 float4  9.0 10.0 11.0 0.0
      uniform 12 float4  0.0  0.0  0.0 0.0
      draw quad
      probe all rgba (192.0, 192.0, 192.0, 192.0)

      I.e., I want to check what happens for a non-square matrix whose smallest dimension is not 1.

      Edited by Giovanni Mascellani
  • Nikolay Sivov added 20 commits

    added 20 commits

    Compare with previous version

  • Nikolay Sivov added 1 commit

    added 1 commit

    • 8ed15e93 - vkd3d-shader/hlsl: Add determinant() function.

    Compare with previous version

  • Nikolay Sivov added 145 commits

    added 145 commits

    Compare with previous version

  • Nikolay Sivov added 1 commit

    added 1 commit

    • 267c09d0 - vkd3d-shader/hlsl: Add determinant() function.

    Compare with previous version

  • Author Developer

    I pushed an update that's using more compact matrix swizzle access, that was fixed since the initial version. I'll happily move this to position printf formatting, once it's merged from mentioned @zfigura's commits.

  • Nikolay Sivov added 832 commits

    added 832 commits

    Compare with previous version

    • Friendly reminder that from my point of view this still needs to have float and half variants depending on the argument type. I don't care too much about the printf() variant that is used, but I would like to see half treated consistently with how we do in (most of) the rest of the HLSL compiler.

    • Author Developer

      Thanks for the reminder. Updated now.

    • Please register or sign in to reply
  • Nikolay Sivov added 51 commits

    added 51 commits

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading