Skip to content
Snippets Groups Projects

Draft: vkd3d-shader/hlsl: Fix assumptions in add_load_index() and add_load_component().

Closed Francisco Casas requested to merge fcasas/vkd3d:fixes3 into master
1 unresolved thread

Currently, we can't use add_load_index() or add_load_component() on the loads to the "retval" variables that come from add_call() because these functions assume that the loaded value (var_instr) won't change between its location and the location and the new load to be created.

We can't get rid of this assumptions either, because, at least add_load_index() may be used in the lhs of an assignment, and in that case we can't store to the "deref" synthetic:

x[0] = 20;

Here I implemented the alternative solution of copying "retval" into a synthetic variable after each function call, but we may want to discuss this approach.

Edited by Francisco Casas

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
  • Francisco Casas changed the description

    changed the description

  • I really don't like this solution, it feels like spaghetti—working around an architecturally broken bit of code by making it worse.

    If we're passing a hlsl_ir_node to add_load_index() then it should be making use of that node's SSA value, not reaching through to the underlying variable.

    Also, I think this is a problem we should deal with for other reasons. For a dumb contrived example, this actually works with native and does what it looks like:

    float4 x; x.yxwz[2] = 1;

    I hate to suggest it, but if we need that unwrapping logic on the LHS of an assignment, I think the most safe and maintainable thing to do is have an instr type that acts as a deref on any source type. Once again, ironically, that's what HLSL_IR_LOAD used to do. I guess Matteo was prescient when he designed the compiler like that the first time. I don't know if we want that to be HLSL_IR_LOAD or if we want a separate type that we'll lower to HLSL_IR_LOAD.

  • Author Developer

    Okay, I agree that the solution in 2/3 is a workaround of a more complex underlying issue, and it will take some time to write a proper solution. So I will separate 3/3 into another MR, because it solves a different bug.

  • Francisco Casas marked this merge request as draft

    marked this merge request as draft

  • Francisco Casas changed title from vkd3d-shader/hlsl: Two bug fixes. to Draft: vkd3d-shader/hlsl: {+Fix assumptions in add_load_index() or add_load_component()+}.

    changed title from vkd3d-shader/hlsl: Two bug fixes. to Draft: vkd3d-shader/hlsl: {+Fix assumptions in add_load_index() or add_load_component()+}.

  • Francisco Casas changed the description

    changed the description

  • Francisco Casas changed title from Draft: vkd3d-shader/hlsl: Fix assumptions in {--}add_load_index(){- or -}add_load_component(){--}. to Draft: vkd3d-shader/hlsl: Fix assumptions in add_load_index() and add_load_component().

    changed title from Draft: vkd3d-shader/hlsl: Fix assumptions in {--}add_load_index(){- or -}add_load_component(){--}. to Draft: vkd3d-shader/hlsl: Fix assumptions in add_load_index() and add_load_component().

  • Francisco Casas mentioned in merge request !96 (merged)

    mentioned in merge request !96 (merged)

  • Author Developer

    After thinking about this for a while, and grabbing some ideas from when we discussed this issue, I think that we should split HLSL_IR_LOAD into two:

    • HLSL_IR_LOAD: represents a load from a variable to a temp, cannot be used as an lhs.
    • HLSL_IR_REFERENCE: Merely contains a deref to a variable (or a part of it) but doesn't have an SSA value and cannot be used as a source.

    So, while parsing we always work with an HLSL_IR_REFERENCE, until a value is required (for instance, as part of an expression), in which case, an HLSL_IR_LOAD has to be created from the HLSL_IR_REFERENCE (basically just copying the deref).

    HLSL_IR_REFERENCE would probably also need an additional field with resource locations for creating HLSL_IR_RESOURCE_LOAD and HLSL_IR_RESOURCE_STORE, when a Texture or UAV is indexed. This could also be the solution for SM 5.1 resource arrays.

    A side-effect of these changes would be changing all the

    /* Only HLSL_IR_LOAD can return an object. */

    comments to

    /* Only HLSL_IR_REFERENCE can return an object. */

    or would allow to remove them.

    I am currently trying this approach.

    • After thinking about this for a while, and grabbing some ideas from when we discussed this issue, I think that we should split HLSL_IR_LOAD into two:

      • HLSL_IR_LOAD: represents a load from a variable to a temp, cannot be used as an lhs.
      • HLSL_IR_REFERENCE: Merely contains a deref to a variable (or a part of it) but doesn't have an SSA value and cannot be used as a source.

      Well, it'd have to have an SSA value. E.g. you can have expressions like "x[1].xyzw".

      HLSL_IR_REFERENCE isn't a bad name, I'll also throw out HLSL_IR_INDEX [to match e.g. hlsl_new_load_index().]

      So, while parsing we always work with an HLSL_IR_REFERENCE, until a value is required (for instance, as part of an expression), in which case, an HLSL_IR_LOAD has to be created from the HLSL_IR_REFERENCE (basically just copying the deref).

      I'd just do them all at once in a pass immediately after parsing; that way you don't have to worry about different instruction types.

      HLSL_IR_REFERENCE would probably also need an additional field with resource locations for creating HLSL_IR_RESOURCE_LOAD and HLSL_IR_RESOURCE_STORE, when a Texture or UAV is indexed. This could also be the solution for SM 5.1 resource arrays.

      Wait, why?

    • Author Developer

      Well, it'd have to have an SSA value. E.g. you can have expressions like "x[1].xyzw".

      Hmmp, in this case x would be an HLSL_IR_REFERENCE, and then x[1] would be another HLSL_IR_REFERENCE (note that x[1] doesn't point to x, because, to create x[1] the deref is copied and then an src to the 1 node is appended to it).

      Then, the swizzle should indeed have point to x[1], but there are either 2 possibilities:

      • If this is in a lhs, we should invert the swizzle into the rhs as we are doing now, leaving only the HLSL_IR_REFERENCE at the lhs (in case there are multiple swizzles, we fold them).
      • If this is an rhs or part of an expression, then x[1] must be turned into a HLSL_IR_LOAD in order to be used by the swizzle. The "evaluation" function that replaces the HLSL_IR_REFERENCE with HLSL_IR_LOADS should also consider this. A scary alternative: adding swizzles to the derefs.

      HLSL_IR_REFERENCE isn't a bad name, I'll also throw out HLSL_IR_INDEX [to match e.g. hlsl_new_load_index().]

      I am not sure about HLSL_IR_INDEX, at first I interpreted it as a node that references 2 nodes: A value node and an index node (x and 1 for x[1]), I am not sure if that was the intent. I tried to implement that, but gets complicated fast (and it doesn't cover the base case: the reference of the variable alone, x). I realized that is far simpler to have the whole deref into one node (basically the same way as HLSL_IR_LOAD).

      HLSL_IR_REFERENCE would probably also need an additional field with resource locations for creating HLSL_IR_RESOURCE_LOAD and HLSL_IR_RESOURCE_STORE, when a Texture or UAV is indexed. This could also be the solution for SM 5.1 resource arrays.

      Wait, why?

      I mean, it is not mandatory, but at the moment seems like a logical step.

      Currently we are using an HLSL_IR_LOAD to initialize the lhs of an HLSL_IR_STORE, but we only care about the deref, and now HLSL_IR_REFERENCE will care about that. In the same way, we are currently using an HLSL_IR_RESOURCE_LOAD's deref and coords to initialize HLSL_IR_RESOURCE_STORE, but not the HLSL_IR_RESOURCE_LOAD itself; so we could use HLSL_IR_REFERENCE for that too, adding the additional fields.

    • HLSL_IR_REFERENCE isn't a bad name, I'll also throw out HLSL_IR_INDEX [to match e.g. hlsl_new_load_index().]

      I am not sure about HLSL_IR_INDEX, at first I interpreted it as a node that references 2 nodes: A value node and an index node (x and 1 for x[1]), I am not sure if that was the intent. I tried to implement that, but gets complicated fast (and it doesn't cover the base case: the reference of the variable alone, x). I realized that is far simpler to have the whole deref into one node (basically the same way as HLSL_IR_LOAD).

      That's exactly my intent, though. The base case could be covered by a union (as with the old HLSL_IR_DEREF) or simply by making it HLSL_IR_LOAD. What complications did you run into?

    • Author Developer

      Adding HLSL_IR_INDEX is not impossible, but some things get complicated, for instance, to build a deref from an index we have to move through the indexes until the base case is found, and then in reverse:

      static bool lower_lhs(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_node *lhs)
      {
          struct hlsl_ir_node *last = NULL, *ptr;
          unsigned int path_len;
      
          hlsl_cleanup_deref(deref);
          /* Traverse index chain from the original load to the lhs node. */
          while (last != lhs)
          {
              path_len = 0;
              ptr = lhs;
              while (ptr->type == HLSL_IR_INDEX)
              {
                  struct hlsl_ir_index *index = hlsl_ir_index(ptr);
      
                  path_len++;
                  assert(index->val);
                  if (index->val == last);
                      break;
                  ptr = index->val;
              }
              last = ptr;
      
              switch (ptr->type)
              {
                  case HLSL_IR_LOAD:
                  {
                      struct hlsl_ir_load *load = hlsl_ir_load(ptr);
      
                      assert(load->src.path_len == 0);
                      hlsl_init_deref(deref, load->src.var, path_len);
                      break;
                  }
      
                  case HLSL_IR_INDEX:
                  {
                      struct hlsl_ir_index *index = hlsl_ir_index(ptr);
                      unsigned int i = deref->path_len - path_len;
      
                      hlsl_src_from_node(&deref->path[i], index->idx.node);
                  }
      
                  default:
                  {
                      hlsl_error(ctx, &lhs->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_LVALUE, "Invalid lvalue.");
                      hlsl_cleanup_deref(deref);
                      return false;
                  }
              }
          }
      
          return true;
      }

      but I think that the base case issue was what throw me off, because the natural solution for me was having to add a new node (using HLSL_IR_LOAD would kill the purpose of separating between mere references or loads (that have an SSA value), unless we add a bool field to differentiate the cases.

      I have to check the old HLSL_IR_DEREF and the union you mention.

    • Adding HLSL_IR_INDEX is not impossible, but some things get complicated, for instance, to build a deref from an index we have to move through the indexes until the base case is found, and then in reverse:

      Actually I believe that lower_lhs() can't work as given, for the exact reason that this merge request exists: you can have sequences like

      2: index a[0] 3: store a[0] = b 4: index @2[0]

      It's basically necessary to do it one step at a time, and synthesize a temp at every step.

      but I think that the base case issue was what throw me off, because the natural solution for me was having to add a new node (using HLSL_IR_LOAD would kill the purpose of separating between mere references or loads (that have an SSA value), unless we add a bool field to differentiate the cases.

      I don't have strong feelings about it. I don't dislike HLSL_IR_LOAD—the invariant then becomes "we never generate a load at parse time with a path length greater than zero, or at least we never return one from the unary_expr rule". But I don't hate the idea of avoiding HLSL_IR_LOAD at parse time completely either.

      I have to check the old HLSL_IR_DEREF and the union you mention.

      Fundamentally it was something like

      struct hlsl_ir_deref { struct hlsl_ir_node node; bool is_var; union { struct hlsl_ir_var *var; struct { struct hlsl_src base; struct hlsl_src offset; } array; } u; };

    • Please register or sign in to reply
  • Francisco Casas mentioned in merge request !124 (merged)

    mentioned in merge request !124 (merged)

  • Author Developer

    Closing, because it is addressed by !124 (merged).

Please register or sign in to reply
Loading