Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Translate copy propagation to index paths. Check for non-static object references. Implicit array initialization.

Merged Francisco Casas requested to merge fcasas/vkd3d:index_paths_part2 into master

Copy propagation is translated to index paths, invalidation of variable components is made more precise.

This, together with checking for non-static object references (which are not allowed in HLSL) allows to set object register sizes back to zero without overlapping registers.

I also included implicit array initialization, since, if I remember correctly, what was holding it back was that structs/arrays with object components couldn't be represented with the correct register offsets.

After this, we have to decide if (and how to) move register allocation and register offset calculation to each shader model.

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

    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/11/22 15:26, Francisco Casas wrote:
      > diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
      > index f076f9cf..cb04d33e 100644
      > --- a/libs/vkd3d-shader/hlsl.c
      > +++ b/libs/vkd3d-shader/hlsl.c
      > @@ -1544,6 +1544,10 @@ static void dump_ir_constant(struct vkd3d_string_buffer *buffer, const struct hl
      >                   vkd3d_string_buffer_printf(buffer, "%.8e ", value->f);
      >                   break;
      >   
      > +            case HLSL_TYPE_HALF:
      > +                vkd3d_string_buffer_printf(buffer, "%.4e ", value->f);
      > +                break;
      > +
      >               case HLSL_TYPE_INT:
      >                   vkd3d_string_buffer_printf(buffer, "%d ", value->i);
      >                   break;
      
      The full 32 bits of precision are encoded into the shader, so I think it 
      makes more sense just to treat halves identically to floats here (and in 
      almost all places, really.)
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/11/22 15:26, Francisco Casas wrote:
      > diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
      > index a47e3d2d..f076f9cf 100644
      > --- a/libs/vkd3d-shader/hlsl.c
      > +++ b/libs/vkd3d-shader/hlsl.c
      > @@ -558,15 +558,18 @@ unsigned int hlsl_type_component_count(const struct hlsl_type *type)
      >       {
      >           return hlsl_type_component_count(type->e.array.type) * type->e.array.elements_count;
      >       }
      > -    if (type->type != HLSL_CLASS_STRUCT)
      > +    if (type->type == HLSL_CLASS_OBJECT)
      >       {
      > -        ERR("Unexpected data type %#x.\n", type->type);
      > -        return 0;
      > +        return 1;
      >       }
      > -
      > -    for (i = 0; i < type->e.record.field_count; ++i)
      > -        count += hlsl_type_component_count(type->e.record.fields[i].type);
      > -    return count;
      > +    if (type->type == HLSL_CLASS_STRUCT)
      > +    {
      > +        for (i = 0; i < type->e.record.field_count; ++i)
      > +            count += hlsl_type_component_count(type->e.record.fields[i].type);
      > +        return count;
      > +    }
      > +    assert(0);
      > +    return 0;
      >   }
      
      This function is ripe for a switch statement.
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/11/22 15:26, Francisco Casas wrote:
      > From: Francisco Casas <fcasas@codeweavers.com>
      > 
      > Signed-off-by: Francisco Casas <fcasas@codeweavers.com>
      > ---
      >   libs/vkd3d-shader/hlsl.c         | 17 +++++++++
      >   libs/vkd3d-shader/hlsl.h         |  1 +
      >   libs/vkd3d-shader/hlsl_codegen.c | 63 ++++++++++++++++++++++++++------
      >   3 files changed, 69 insertions(+), 12 deletions(-)
      
      Nice :D
      
      > +unsigned int hlsl_type_length(const struct hlsl_type *type)
      
      "length" feels a bit ambiguous (I thought for a second it might mean 
      "component count"). How about "element count"?
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/11/22 15:26, Francisco Casas wrote:
      > +static bool check_for_non_static_object_references(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr,
      > +        void *context)
      
      Just a suggestion, I think "validate" would be a good word to use here.
      
      > +{
      > +    unsigned int start, count;
      > +    bool invalid = false;
      > +
      > +    if (instr->type == HLSL_IR_RESOURCE_LOAD)
      > +    {
      > +        struct hlsl_ir_resource_load *load = hlsl_ir_resource_load(instr);
      > +
      > +        invalid |= !hlsl_component_index_range_from_deref(ctx, &load->resource, &start, &count);
      > +        if (load->sampler.var)
      > +            invalid |= !hlsl_component_index_range_from_deref(ctx, &load->sampler, &start, &count);
      > +    }
      > +    else if (instr->type == HLSL_IR_LOAD && instr->data_type->type == HLSL_CLASS_OBJECT)
      > +    {
      > +        struct hlsl_ir_load *load = hlsl_ir_load(instr);
      > +
      > +        invalid |= !hlsl_component_index_range_from_deref(ctx, &load->src, &start, &count);
      
      Do we need this "else if" branch at all? In fact, won't this result in 
      multiple messages if a dynamic dereference is used?
      
      > +    }
      > +
      > +    if (invalid)
      > +        hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF,
      > +                "All object references must be determinable at compile time.");
      
      I think it'd be helpful to name the specific expression that can't be 
      resolved (and, along the same lines, to report resources and samplers 
      separately).
      
      > +
      > +    return invalid;
      
      Shouldn't this always return false? We don't have any more work to do.
      
      > +}
      > +
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/11/22 15:26, Francisco Casas wrote:
      > +[texture 0]
      > +size (1, 1)
      > +1.0 1.0 1.0 1.0
      > +
      > +[texture 1]
      > +size (1, 1)
      > +2.0 2.0 2.0 1.0
      > +
      > +[texture 2]
      > +size (1, 1)
      > +3.0 3.0 3.0 1.0
      > +
      > +[pixel shader]
      > +Texture2D tex[3];
      > +
      > +struct foo {
      > +    float4 p;
      > +    Texture2D t;
      > +};
      > +
      > +float4 main() : sv_target
      > +{
      > +    struct foo s[3];
      > +
      > +    s[0].t = tex[0];
      > +    s[1].t = tex[1];
      > +    s[2].t = tex[2];
      > +    return 100 * s[2].t.Load(0) + 10 * s[0].t.Load(0) + s[1].t.Load(0);
      > +}
      > +
      > +[test]
      > +todo draw quad
      > +todo probe all rgba (312, 312, 312, 111)
      > +
      > +
      > +[texture 0]
      > +size (1, 1)
      > +1.0 1.0 1.0 1.0
      > +
      > +[texture 1]
      > +size (1, 1)
      > +2.0 2.0 2.0 1.0
      > +
      > +[texture 2]
      > +size (1, 1)
      > +3.0 3.0 3.0 1.0
      > +
      
      You don't need to redeclare the textures if they don't change.
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Francisco Casas replied on the mailing list:

      Okay, so, just to confirm and for future reference, at least in the HLSL 
      compiler we are calling:
      
      "component": To an "atomic", single type that cannot be indexed, i.e. a 
      scalar or an object[1], within another type, at any nesting level.
      
      "element": To one of the outermost parts of another type:
      * A field within a struct.
      * An array element within an array.
      * A column/row within a matrix.
      * A scalar within a vector.
      i.e. the resulting type after applying one indexation, going down one 
      nesting level.
      
      ---
      
      [1] Textures can be indexed in HLSL, but we are handling those cases as 
      a resource_load/resource_store. I am not sure if this would also apply 
      for Shader Model 5.1 resource arrays.
      
      
      On 12-08-22 14:06, Zebediah Figura (she/her) wrote:
      > On 8/11/22 15:26, Francisco Casas wrote:
      >> From: Francisco Casas <fcasas@codeweavers.com>
      >>
      >> Signed-off-by: Francisco Casas <fcasas@codeweavers.com>
      >> ---
      >>   libs/vkd3d-shader/hlsl.c         | 17 +++++++++
      >>   libs/vkd3d-shader/hlsl.h         |  1 +
      >>   libs/vkd3d-shader/hlsl_codegen.c | 63 ++++++++++++++++++++++++++------
      >>   3 files changed, 69 insertions(+), 12 deletions(-)
      > 
      > Nice :D
      > 
      >> +unsigned int hlsl_type_length(const struct hlsl_type *type)
      > 
      > "length" feels a bit ambiguous (I thought for a second it might mean 
      > "component count"). How about "element count"?
      > _______________________________________________
      > wine-gitlab mailing list -- wine-gitlab@winehq.org
      > To unsubscribe send an email to wine-gitlab-leave@winehq.org
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/16/22 11:04, Francisco Casas wrote:
      > Okay, so, just to confirm and for future reference, at least in the HLSL 
      > compiler we are calling:
      > 
      > "component": To an "atomic", single type that cannot be indexed, i.e. a 
      > scalar or an object[1], within another type, at any nesting level.
      > 
      > "element": To one of the outermost parts of another type:
      > * A field within a struct.
      > * An array element within an array.
      > * A column/row within a matrix.
      > * A scalar within a vector.
      > i.e. the resulting type after applying one indexation, going down one 
      > nesting level.
      
      Right, that feels intuitive enough to me. (But we can revisit the 
      terminology if others are having trouble with it.)
      
      > 
      > ---
      > 
      > [1] Textures can be indexed in HLSL, but we are handling those cases as 
      > a resource_load/resource_store. I am not sure if this would also apply 
      > for Shader Model 5.1 resource arrays.
      
      Yeah, I don't think either one of those should be an array index as far 
      as the IR is concerned.
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Francisco Casas replied on the mailing list:

      Hello,
      
      On 12-08-22 14:06, Zebediah Figura (she/her) wrote:
      > On 8/11/22 15:26, Francisco Casas wrote:
      >> +static bool check_for_non_static_object_references(struct hlsl_ctx 
      >> *ctx, struct hlsl_ir_node *instr,
      >> +        void *context)
      > 
      > Just a suggestion, I think "validate" would be a good word to use here.
      > 
      
      Okay, going for 'validate_static_object_references'.
      
      >> +{
      >> +    unsigned int start, count;
      >> +    bool invalid = false;
      >> +
      >> +    if (instr->type == HLSL_IR_RESOURCE_LOAD)
      >> +    {
      >> +        struct hlsl_ir_resource_load *load = 
      >> hlsl_ir_resource_load(instr);
      >> +
      >> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >> &load->resource, &start, &count);
      >> +        if (load->sampler.var)
      >> +            invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >> &load->sampler, &start, &count);
      >> +    }
      >> +    else if (instr->type == HLSL_IR_LOAD && instr->data_type->type == 
      >> HLSL_CLASS_OBJECT)
      >> +    {
      >> +        struct hlsl_ir_load *load = hlsl_ir_load(instr);
      >> +
      >> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >> &load->src, &start, &count);
      > 
      > Do we need this "else if" branch at all? In fact, won't this result in 
      > multiple messages if a dynamic dereference is used?
      > 
      
      It indeed results in multiple messages. However, I think we may need 
      both branches:
      
      
      On one hand, the native compiler throws an exception on dynamic object 
      references even if they are not used, e.g. with the following ps_5_0 shader:
      ---
      uniform int p;
      Texture2D tex[2];
      
      float4 main() : sv_target
      {
           Texture2D u = tex[p]; // Not used, but throws an exception anyways.
      
           return float4(0, 1, 2, 3);
      }
      ---
      which makes necessary the "else if" branch (and to do this validation 
      pass before DCE, as we already do).
      
      On the other hand, if we delete the "if" branch we would be introducing 
      the assumption that for each resource load there is an object load that 
      will trigger the error (same for samplers), which seems a little hard to 
      remember to maintain this way if we introduce changes in the previous 
      passes. Furthermore, I am not totally sure if it always holds true **now**.
      
      >> +    }
      >> +
      >> +    if (invalid)
      >> +        hlsl_error(ctx, &instr->loc, 
      >> VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF,
      >> +                "All object references must be determinable at 
      >> compile time.");
      > 
      > I think it'd be helpful to name the specific expression that can't be 
      > resolved (and, along the same lines, to report resources and samplers 
      > separately).
      > 
      
      Okay. I am thinking on using hlsl_note() after hlsl_error() to report 
      the location of the expression to the user, getting outputs like:
      ---
      testm.hlsl:7:20: E5022: Loaded resource must be determinable at compile 
      time.
      testm.hlsl:7:14: Expression cannot be resolved statically.
      ---
      Is this okay?
      
      I can also print the instruction pointer as a TRACE() or similar.
      
      >> +
      >> +    return invalid;
      > 
      > Shouldn't this always return false? We don't have any more work to do.
      > 
      
      Ah right, I wasn't following that convention. I just thought that having 
      "invalid" as return value could be useful. I will make this function 
      return "false" always.
      
      >> +}
      >> +
      > _______________________________________________
      > wine-gitlab mailing list -- wine-gitlab@winehq.org
      > To unsubscribe send an email to wine-gitlab-leave@winehq.org
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/16/22 16:13, Francisco Casas wrote:
      >>> +{
      >>> +    unsigned int start, count;
      >>> +    bool invalid = false;
      >>> +
      >>> +    if (instr->type == HLSL_IR_RESOURCE_LOAD)
      >>> +    {
      >>> +        struct hlsl_ir_resource_load *load = 
      >>> hlsl_ir_resource_load(instr);
      >>> +
      >>> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>> &load->resource, &start, &count);
      >>> +        if (load->sampler.var)
      >>> +            invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>> &load->sampler, &start, &count);
      >>> +    }
      >>> +    else if (instr->type == HLSL_IR_LOAD && instr->data_type->type 
      >>> == HLSL_CLASS_OBJECT)
      >>> +    {
      >>> +        struct hlsl_ir_load *load = hlsl_ir_load(instr);
      >>> +
      >>> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>> &load->src, &start, &count);
      >>
      >> Do we need this "else if" branch at all? In fact, won't this result in 
      >> multiple messages if a dynamic dereference is used?
      >>
      > 
      > It indeed results in multiple messages. However, I think we may need 
      > both branches:
      > 
      > 
      > On one hand, the native compiler throws an exception on dynamic object 
      > references even if they are not used, e.g. with the following ps_5_0 
      > shader:
      > ---
      > uniform int p;
      > Texture2D tex[2];
      > 
      > float4 main() : sv_target
      > {
      >      Texture2D u = tex[p]; // Not used, but throws an exception anyways.
      > 
      >      return float4(0, 1, 2, 3);
      > }
      > ---
      > which makes necessary the "else if" branch (and to do this validation 
      > pass before DCE, as we already do).
      > 
      > On the other hand, if we delete the "if" branch we would be introducing 
      > the assumption that for each resource load there is an object load that 
      > will trigger the error (same for samplers), which seems a little hard to 
      > remember to maintain this way if we introduce changes in the previous 
      > passes. Furthermore, I am not totally sure if it always holds true **now**.
      
      Urgh, that is awkward.
      
      I don't think it should block this patch (or maybe it should block this 
      patch but not this series? Can we skip this one and keep the relevant 
      tests todo?) but I also don't like the idea of generating duplicate 
      error lines.
      
      One obvious way to avoid that would be to do constant folding at parse 
      time (where necessary) and then just generate error messages at parse 
      time. We probably want that anyway in some places, given things like 
      hlsl-array-dimension.shader_test.
      
      That said, interesting fact: if you replace the line
      
           Texture2D u = tex[p];
      
      with
      
           tex[p];
      
      the shader does compile. (As an interesting footnote, you can get a 
      similar effect with expressions like "1 / 0".)
      
      Which almost suggests that we could DCE everything but stores, and then 
      warn as-is...
      
      > 
      >>> +    }
      >>> +
      >>> +    if (invalid)
      >>> +        hlsl_error(ctx, &instr->loc, 
      >>> VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF,
      >>> +                "All object references must be determinable at 
      >>> compile time.");
      >>
      >> I think it'd be helpful to name the specific expression that can't be 
      >> resolved (and, along the same lines, to report resources and samplers 
      >> separately).
      >>
      > 
      > Okay. I am thinking on using hlsl_note() after hlsl_error() to report 
      > the location of the expression to the user, getting outputs like:
      > ---
      > testm.hlsl:7:20: E5022: Loaded resource must be determinable at compile 
      > time.
      > testm.hlsl:7:14: Expression cannot be resolved statically.
      > ---
      > Is this okay?
      > 
      > I can also print the instruction pointer as a TRACE() or similar.
      
      I think my comment as-is made no sense or was miswritten; we already 
      print the load location. (Is there another location we should be 
      printing?) What I was probably intending was that we should print the 
      variable name; I think that would make sense. We shouldn't need a 
      separate hlsl_note() to do so though.
      
      > 
      >>> +
      >>> +    return invalid;
      >>
      >> Shouldn't this always return false? We don't have any more work to do.
      >>
      > 
      > Ah right, I wasn't following that convention. I just thought that having 
      > "invalid" as return value could be useful. I will make this function 
      > return "false" always.
      
      Right. We're not running this function in a loop (or using the return 
      value at all) but I think it makes sense to be consistent about the 
      return value's usage.
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Francisco Casas replied on the mailing list:

      On 16-08-22 20:14, Zebediah Figura (she/her) wrote:
      > On 8/16/22 16:13, Francisco Casas wrote:
      >>>> +{
      >>>> +    unsigned int start, count;
      >>>> +    bool invalid = false;
      >>>> +
      >>>> +    if (instr->type == HLSL_IR_RESOURCE_LOAD)
      >>>> +    {
      >>>> +        struct hlsl_ir_resource_load *load = 
      >>>> hlsl_ir_resource_load(instr);
      >>>> +
      >>>> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>>> &load->resource, &start, &count);
      >>>> +        if (load->sampler.var)
      >>>> +            invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>>> &load->sampler, &start, &count);
      >>>> +    }
      >>>> +    else if (instr->type == HLSL_IR_LOAD && instr->data_type->type 
      >>>> == HLSL_CLASS_OBJECT)
      >>>> +    {
      >>>> +        struct hlsl_ir_load *load = hlsl_ir_load(instr);
      >>>> +
      >>>> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>>> &load->src, &start, &count);
      >>>
      >>> Do we need this "else if" branch at all? In fact, won't this result 
      >>> in multiple messages if a dynamic dereference is used?
      >>>
      >>
      >> It indeed results in multiple messages. However, I think we may need 
      >> both branches:
      >>
      >>
      >> On one hand, the native compiler throws an exception on dynamic object 
      >> references even if they are not used, e.g. with the following ps_5_0 
      >> shader:
      >> ---
      >> uniform int p;
      >> Texture2D tex[2];
      >>
      >> float4 main() : sv_target
      >> {
      >>      Texture2D u = tex[p]; // Not used, but throws an exception anyways.
      >>
      >>      return float4(0, 1, 2, 3);
      >> }
      >> ---
      >> which makes necessary the "else if" branch (and to do this validation 
      >> pass before DCE, as we already do).
      >>
      >> On the other hand, if we delete the "if" branch we would be 
      >> introducing the assumption that for each resource load there is an 
      >> object load that will trigger the error (same for samplers), which 
      >> seems a little hard to remember to maintain this way if we introduce 
      >> changes in the previous passes. Furthermore, I am not totally sure if 
      >> it always holds true **now**.
      > 
      > Urgh, that is awkward.
      > 
      > I don't think it should block this patch (or maybe it should block this 
      > patch but not this series? Can we skip this one and keep the relevant 
      > tests todo?) but I also don't like the idea of generating duplicate 
      > error lines.
      > 
      > One obvious way to avoid that would be to do constant folding at parse 
      > time (where necessary) and then just generate error messages at parse 
      > time. We probably want that anyway in some places, given things like 
      > hlsl-array-dimension.shader_test.
      > 
      > That said, interesting fact: if you replace the line
      > 
      >      Texture2D u = tex[p];
      > 
      > with
      > 
      >      tex[p];
      > 
      > the shader does compile. (As an interesting footnote, you can get a 
      > similar effect with expressions like "1 / 0".)
      > 
      > Which almost suggests that we could DCE everything but stores, and then 
      > warn as-is...
      > 
      
      I am not sure we should allow stores to survive DCE, for instance, if 
      the store is not used, but it is static:
      
      ---
      Texture2D tex[2];
      
      float4 main() : sv_target
      {
           Texture2D u = tex[0]; // Store is deleted.
      
           return float4(0, 1, 2, 3);
      }
      ---
      
      the shader compiles in native, and ignores the store:
      
      ---
           ps_3_0
           def c0, 0, 1, 2, 3
           mov oC0, c0
      ---
      
      
      After some thinking, I think the following idea is worth trying:
      
      Moving the "else if" branch to dce, and only for nodes that are being 
      deleted (because they are not used in a resource_load).
      
      
      ...
      
      Btw, I haven't think this too much yet, but, if the resource_load·s 
      pointed to load nodes (both in resource and sampler) instead of having 
      direct derefs, this validation pass with only the "else if" branch 
      should be enough.
      
      However, if we consider this change seriously, I think it will be better 
      to introduce it after this patch series.
      
      >>
      >>>> +    }
      >>>> +
      >>>> +    if (invalid)
      >>>> +        hlsl_error(ctx, &instr->loc, 
      >>>> VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF,
      >>>> +                "All object references must be determinable at 
      >>>> compile time.");
      >>>
      >>> I think it'd be helpful to name the specific expression that can't be 
      >>> resolved (and, along the same lines, to report resources and samplers 
      >>> separately).
      >>>
      >>
      >> Okay. I am thinking on using hlsl_note() after hlsl_error() to report 
      >> the location of the expression to the user, getting outputs like:
      >> ---
      >> testm.hlsl:7:20: E5022: Loaded resource must be determinable at 
      >> compile time.
      >> testm.hlsl:7:14: Expression cannot be resolved statically.
      >> ---
      >> Is this okay?
      >>
      >> I can also print the instruction pointer as a TRACE() or similar.
      > 
      > I think my comment as-is made no sense or was miswritten; we already 
      > print the load location. (Is there another location we should be 
      > printing?) What I was probably intending was that we should print the 
      > variable name; I think that would make sense. We shouldn't need a 
      > separate hlsl_note() to do so though.
      > 
      
      I thought you were referring to the location of the specific node (or 
      nodes) in the index path that are not constant. But yes, I think that 
      the load location is enough. I will add the variable name in the message.
      
      >>
      >>>> +
      >>>> +    return invalid;
      >>>
      >>> Shouldn't this always return false? We don't have any more work to do.
      >>>
      >>
      >> Ah right, I wasn't following that convention. I just thought that 
      >> having "invalid" as return value could be useful. I will make this 
      >> function return "false" always.
      > 
      > Right. We're not running this function in a loop (or using the return 
      > value at all) but I think it makes sense to be consistent about the 
      > return value's usage.
      > _______________________________________________
      > wine-gitlab mailing list -- wine-gitlab@winehq.org
      > To unsubscribe send an email to wine-gitlab-leave@winehq.org
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/17/22 15:21, Francisco Casas wrote:
      > On 16-08-22 20:14, Zebediah Figura (she/her) wrote:
      >> On 8/16/22 16:13, Francisco Casas wrote:
      >>>>> +{
      >>>>> +    unsigned int start, count;
      >>>>> +    bool invalid = false;
      >>>>> +
      >>>>> +    if (instr->type == HLSL_IR_RESOURCE_LOAD)
      >>>>> +    {
      >>>>> +        struct hlsl_ir_resource_load *load = 
      >>>>> hlsl_ir_resource_load(instr);
      >>>>> +
      >>>>> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>>>> &load->resource, &start, &count);
      >>>>> +        if (load->sampler.var)
      >>>>> +            invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>>>> &load->sampler, &start, &count);
      >>>>> +    }
      >>>>> +    else if (instr->type == HLSL_IR_LOAD && instr->data_type->type 
      >>>>> == HLSL_CLASS_OBJECT)
      >>>>> +    {
      >>>>> +        struct hlsl_ir_load *load = hlsl_ir_load(instr);
      >>>>> +
      >>>>> +        invalid |= !hlsl_component_index_range_from_deref(ctx, 
      >>>>> &load->src, &start, &count);
      >>>>
      >>>> Do we need this "else if" branch at all? In fact, won't this result 
      >>>> in multiple messages if a dynamic dereference is used?
      >>>>
      >>>
      >>> It indeed results in multiple messages. However, I think we may need 
      >>> both branches:
      >>>
      >>>
      >>> On one hand, the native compiler throws an exception on dynamic 
      >>> object references even if they are not used, e.g. with the following 
      >>> ps_5_0 shader:
      >>> ---
      >>> uniform int p;
      >>> Texture2D tex[2];
      >>>
      >>> float4 main() : sv_target
      >>> {
      >>>      Texture2D u = tex[p]; // Not used, but throws an exception anyways.
      >>>
      >>>      return float4(0, 1, 2, 3);
      >>> }
      >>> ---
      >>> which makes necessary the "else if" branch (and to do this validation 
      >>> pass before DCE, as we already do).
      >>>
      >>> On the other hand, if we delete the "if" branch we would be 
      >>> introducing the assumption that for each resource load there is an 
      >>> object load that will trigger the error (same for samplers), which 
      >>> seems a little hard to remember to maintain this way if we introduce 
      >>> changes in the previous passes. Furthermore, I am not totally sure if 
      >>> it always holds true **now**.
      >>
      >> Urgh, that is awkward.
      >>
      >> I don't think it should block this patch (or maybe it should block 
      >> this patch but not this series? Can we skip this one and keep the 
      >> relevant tests todo?) but I also don't like the idea of generating 
      >> duplicate error lines.
      >>
      >> One obvious way to avoid that would be to do constant folding at parse 
      >> time (where necessary) and then just generate error messages at parse 
      >> time. We probably want that anyway in some places, given things like 
      >> hlsl-array-dimension.shader_test.
      >>
      >> That said, interesting fact: if you replace the line
      >>
      >>      Texture2D u = tex[p];
      >>
      >> with
      >>
      >>      tex[p];
      >>
      >> the shader does compile. (As an interesting footnote, you can get a 
      >> similar effect with expressions like "1 / 0".)
      >>
      >> Which almost suggests that we could DCE everything but stores, and 
      >> then warn as-is...
      >>
      > 
      > I am not sure we should allow stores to survive DCE, for instance, if 
      > the store is not used, but it is static:
      > 
      > ---
      > Texture2D tex[2];
      > 
      > float4 main() : sv_target
      > {
      >      Texture2D u = tex[0]; // Store is deleted.
      > 
      >      return float4(0, 1, 2, 3);
      > }
      > ---
      > 
      > the shader compiles in native, and ignores the store:
      > 
      > ---
      >      ps_3_0
      >      def c0, 0, 1, 2, 3
      >      mov oC0, c0
      > ---
      > 
      
      We definitely don't want stores to *ultimately* survive DCE, but we 
      could potentially have an early DCE pass that *only* throws away 
      non-stores, so that we can match native behaviour wrt things like this.
      
      But I don't know that it's worth trying that hard to match. I'd frankly 
      rather ignore this quirk and just apply constant folding at parse time, 
      like the first approach I mentioned.
      
      > 
      > After some thinking, I think the following idea is worth trying:
      > 
      > Moving the "else if" branch to dce, and only for nodes that are being 
      > deleted (because they are not used in a resource_load).
      
      Ech, I don't much like that. Too much conflation of different things.
      
      > ...
      > 
      > Btw, I haven't think this too much yet, but, if the resource_load·s 
      > pointed to load nodes (both in resource and sampler) instead of having 
      > direct derefs, this validation pass with only the "else if" branch 
      > should be enough.
      > 
      > However, if we consider this change seriously, I think it will be better 
      > to introduce it after this patch series.
      
      That's been considered but I don't like it; it means that the load nodes 
      will not correspond to an allocated temp but can't be deleted.
      
      > 
      >>>
      >>>>> +    }
      >>>>> +
      >>>>> +    if (invalid)
      >>>>> +        hlsl_error(ctx, &instr->loc, 
      >>>>> VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF,
      >>>>> +                "All object references must be determinable at 
      >>>>> compile time.");
      >>>>
      >>>> I think it'd be helpful to name the specific expression that can't 
      >>>> be resolved (and, along the same lines, to report resources and 
      >>>> samplers separately).
      >>>>
      >>>
      >>> Okay. I am thinking on using hlsl_note() after hlsl_error() to report 
      >>> the location of the expression to the user, getting outputs like:
      >>> ---
      >>> testm.hlsl:7:20: E5022: Loaded resource must be determinable at 
      >>> compile time.
      >>> testm.hlsl:7:14: Expression cannot be resolved statically.
      >>> ---
      >>> Is this okay?
      >>>
      >>> I can also print the instruction pointer as a TRACE() or similar.
      >>
      >> I think my comment as-is made no sense or was miswritten; we already 
      >> print the load location. (Is there another location we should be 
      >> printing?) What I was probably intending was that we should print the 
      >> variable name; I think that would make sense. We shouldn't need a 
      >> separate hlsl_note() to do so though.
      >>
      > 
      > I thought you were referring to the location of the specific node (or 
      > nodes) in the index path that are not constant. But yes, I think that 
      > the load location is enough. I will add the variable name in the message.
      
      Hmm, that's a good idea actually; I forgot we already do have enough 
      information to print that. But it can wait for later, sure.
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
  • Francisco Casas added 11 commits

    added 11 commits

    • 4c7d446e - vkd3d-shader/hlsl: Rename 'inner_type' to 'element_type' in function
    • 527a4005 - vkd3d-shader/hlsl: Emit a fixme on complex resource load object derefs.
    • e42ae0db - tests: Test object references.
    • 34183fb1 - vkd3d-shader/hlsl: Set component count for objects to 1.
    • e88a6bfe - vkd3d-shader/hlsl: Skip implicit conversion if types are equal.
    • a6f9d35d - vkd3d-shader/hlsl: Print halfs in dump_ir_constant().
    • 5a2c586a - vkd3d-shader/hlsl: Replace register offsets with index paths in copy
    • bde11e00 - vkd3d-shader/hlsl: Invalidate components more precisely in copy
    • 598d5a40 - vkd3d-shader/hlsl: Check for non-static object references.
    • 51b22407 - vkd3d-shader/hlsl: Set objects' register size back to 0.
    • 438a4c4b - vkd3d-shader/hlsl: Support initialization of implicit size arrays.

    Compare with previous version

  • Francisco Casas added 9 commits

    added 9 commits

    • fc3f0e8c - tests: Test object references.
    • f877c8ee - vkd3d-shader/hlsl: Set component count for objects to 1.
    • f3998453 - vkd3d-shader/hlsl: Skip implicit conversion if types are equal.
    • 18f5475f - vkd3d-shader/hlsl: Print halfs in dump_ir_constant().
    • b5e807d3 - vkd3d-shader/hlsl: Replace register offsets with index paths in copy
    • ecc35955 - vkd3d-shader/hlsl: Invalidate components more precisely in copy
    • b522c268 - vkd3d-shader/hlsl: Check for non-static object references.
    • 4ecb22ee - vkd3d-shader/hlsl: Set objects' register size back to 0.
    • 3cd70138 - vkd3d-shader/hlsl: Support initialization of implicit size arrays.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 11 commits

    • a557c461 - vkd3d-shader/hlsl: Rename 'inner_type' to 'element_type' in function name.
    • 3a2e5a0f - vkd3d-shader/hlsl: Emit a fixme on complex resource load object derefs.
    • 93a84cb1 - tests: Test object references.
    • eea7958a - vkd3d-shader/hlsl: Set component count for objects to 1.
    • 2204311b - vkd3d-shader/hlsl: Skip implicit conversion if types are equal.
    • d47f462c - vkd3d-shader/hlsl: Print halfs in dump_ir_constant().
    • 879849ac - vkd3d-shader/hlsl: Replace register offsets with index paths in copy propagation.
    • 4063e0fa - vkd3d-shader/hlsl: Invalidate components more precisely in copy propagation.
    • a2e8c504 - vkd3d-shader/hlsl: Check for non-static object references.
    • 5270cb6b - vkd3d-shader/hlsl: Set objects' register size back to 0.
    • dd092540 - vkd3d-shader/hlsl: Support initialization of implicit size arrays.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • added 12 commits

    • 1c61b206 - 1 commit from branch wine:master
    • f843a7ba - vkd3d-shader/hlsl: Rename 'inner_type' to 'element_type' in function name.
    • f3432966 - vkd3d-shader/hlsl: Emit a fixme on complex resource load object derefs.
    • e9829fdc - tests: Test object references.
    • 732f1737 - vkd3d-shader/hlsl: Set component count for objects to 1.
    • a8b77b85 - vkd3d-shader/hlsl: Skip implicit conversion if types are equal.
    • 10bd0c48 - vkd3d-shader/hlsl: Print halfs in dump_ir_constant().
    • b5b08bd8 - vkd3d-shader/hlsl: Replace register offsets with index paths in copy propagation.
    • 1bba18aa - vkd3d-shader/hlsl: Invalidate components more precisely in copy propagation.
    • 6989266e - vkd3d-shader/hlsl: Check for non-static object references.
    • 96a72367 - vkd3d-shader/hlsl: Set objects' register size back to 0.
    • 3d9baef3 - vkd3d-shader/hlsl: Support initialization of implicit size arrays.

    Compare with previous version

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