[PATCH vkd3d v3 02/12] vkd3d-shader/hlsl: Add texel_offset field to hlsl_ir_resource_load.

[Francisco Casas]

Signed-off-by: Francisco Casas 
---
 libs/vkd3d-shader/hlsl.c         |  9 ++++++++-
 libs/vkd3d-shader/hlsl.h         |  3 ++-
 libs/vkd3d-shader/hlsl.y         | 14 ++++++++++----
 libs/vkd3d-shader/hlsl_codegen.c |  2 ++
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index d2ea4c34..b426e461 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -641,7 +641,7 @@ struct hlsl_ir_load *hlsl_new_var_load(struct hlsl_ctx *ctx, struct hlsl_ir_var
 struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, struct hlsl_type *data_type,
         enum hlsl_resource_load_type type, struct hlsl_ir_var *resource, struct hlsl_ir_node *resource_offset,
         struct hlsl_ir_var *sampler, struct hlsl_ir_node *sampler_offset, struct hlsl_ir_node *coords,
-        const struct vkd3d_shader_location *loc)
+        struct hlsl_ir_node *texel_offset, const struct vkd3d_shader_location *loc)
 {
     struct hlsl_ir_resource_load *load;

@@ -654,6 +654,7 @@ struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, struc
     load->sampler.var = sampler;
     hlsl_src_from_node(&load->sampler.offset, sampler_offset);
     hlsl_src_from_node(&load->coords, coords);
+    hlsl_src_from_node(&load->texel_offset, texel_offset);
     return load;
 }

@@ -1284,6 +1285,11 @@ static void dump_ir_resource_load(struct vkd3d_string_buffer *buffer, const stru
     dump_deref(buffer, &load->sampler);
     vkd3d_string_buffer_printf(buffer, ", coords = ");
     dump_src(buffer, &load->coords);
+    if (load->texel_offset.node)
+    {
+        vkd3d_string_buffer_printf(buffer, ", offset = ");
+        dump_src(buffer, &load->texel_offset);
+    }
     vkd3d_string_buffer_printf(buffer, ")");
 }

@@ -1457,6 +1463,7 @@ static void free_ir_resource_load(struct hlsl_ir_resource_load *load)
     hlsl_src_remove(&load->coords);
     hlsl_src_remove(&load->sampler.offset);
     hlsl_src_remove(&load->resource.offset);
+    hlsl_src_remove(&load->texel_offset);
     vkd3d_free(load);
 }

diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index 57acf3a0..2396adb4 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -386,6 +386,7 @@ struct hlsl_ir_resource_load
     enum hlsl_resource_load_type load_type;
     struct hlsl_deref resource, sampler;
     struct hlsl_src coords;
+    struct hlsl_src texel_offset;
 };

struct hlsl_ir_store
@@ -710,7 +711,7 @@ struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_loc
 struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, struct hlsl_type *data_type,
         enum hlsl_resource_load_type type, struct hlsl_ir_var *resource, struct hlsl_ir_node *resource_offset,
         struct hlsl_ir_var *sampler, struct hlsl_ir_node *sampler_offset, struct hlsl_ir_node *coords,
-        const struct vkd3d_shader_location *loc);
+        struct hlsl_ir_node *texel_offset, const struct vkd3d_shader_location *loc);
 struct hlsl_ir_store *hlsl_new_simple_store(struct hlsl_ctx *ctx, struct hlsl_ir_var *lhs, struct hlsl_ir_node *rhs);
 struct hlsl_ir_store *hlsl_new_store(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, struct hlsl_ir_node *offset,
         struct hlsl_ir_node *rhs, unsigned int writemask, struct vkd3d_shader_location loc);
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 636882c4..33bfee5b 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -1875,7 +1875,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
             return false;

if (!(load = hlsl_new_resource_load(ctx, object_type->e.resource_format, HLSL_RESOURCE_LOAD,
-                object_load->src.var, object_load->src.offset.node, NULL, NULL, coords, loc)))
+                object_load->src.var, object_load->src.offset.node, NULL, NULL, coords, NULL, loc)))
             return false;
         list_add_tail(instrs, &load->node.entry);
         return true;
@@ -1885,6 +1885,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
         const unsigned int sampler_dim = sampler_dim_count(object_type->sampler_dim);
         const struct hlsl_type *sampler_type;
         struct hlsl_ir_resource_load *load;
+        struct hlsl_ir_node *offset = NULL;
         struct hlsl_ir_load *sampler_load;
         struct hlsl_ir_node *coords;

@@ -1894,8 +1895,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
                     "Wrong number of arguments to method 'Sample': expected 2 or 3, but got %u.", params->args_count);
             return false;
         }
-        if (params->args_count == 3)
-            FIXME("Ignoring offset parameter.\n");

sampler_type = params->args[0]->data_type;
         if (sampler_type->type != HLSL_CLASS_OBJECT || sampler_type->base_type != HLSL_TYPE_SAMPLER
@@ -1917,9 +1916,16 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
                 hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc)))
             coords = params->args[1];

+        if (params->args_count == 3)
+        {
+            if (!(offset = add_implicit_conversion(ctx, instrs, params->args[2],
+                    hlsl_get_vector_type(ctx, HLSL_TYPE_INT, sampler_dim), loc)))
+                offset = params->args[2];
+        }
+
         if (!(load = hlsl_new_resource_load(ctx, object_type->e.resource_format,
                 HLSL_RESOURCE_SAMPLE, object_load->src.var, object_load->src.offset.node,
-                sampler_load->src.var, sampler_load->src.offset.node, coords, loc)))
+                sampler_load->src.var, sampler_load->src.offset.node, coords, offset, loc)))
             return false;
         list_add_tail(instrs, &load->node.entry);
         return true;
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index 75716bdf..8918dc27 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -964,6 +964,8 @@ static void compute_liveness_recurse(struct hlsl_block *block, unsigned int loop
             }

load->coords.node->last_read = instr->index;
+            if (load->texel_offset.node)
+                load->texel_offset.node->last_read = instr->index;
             break;
         }
         case HLSL_IR_SWIZZLE:

[Zebediah Figura (she/her)]

Signed-off-by: Zebediah Figura

[Giovanni Mascellani]

Hi,

On 17/12/21 20:12, Francisco Casas wrote:
> @@ -1917,9 +1916,16 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
>                   hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc)))
>               coords = params->args[1];
> -
>    if (params->args_count == 3)
> -
>    {
> -
>        if (!(offset = add_implicit_conversion(ctx, instrs, params->args[2],
> -
>                hlsl_get_vector_type(ctx, HLSL_TYPE_INT, sampler_dim), loc)))
> -
>            offset = params->args[2];
> -
>    }
> -

I don't understand the point of "offset = params->args[2]" is the 
implicit conversion fails. Can that failure be recovered later? If not 
(and we are assigning something to offset just to keep compiling and 
produce as much diagnostics as we can), then I'd rather assign NULL to 
avoid further problems. Am I missing something?

Thanks, Giovanni.

[Francisco Casas]

December 20, 2021 8:02 AM, "Giovanni Mascellani"  wrote:
> Hi,
>
> On 17/12/21 20:12, Francisco Casas wrote:
> > @@ -1917,9 +1916,16 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs,
> > struct hl
> > hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc)))
> > coords = params->args[1];
> > > - if (params->args_count == 3)
> > - {
> > - if (!(offset = add_implicit_conversion(ctx, instrs, params->args[2],
> > - hlsl_get_vector_type(ctx, HLSL_TYPE_INT, sampler_dim), loc)))
> > - offset = params->args[2];
> > - }
> > -
>
> I don't understand the point of "offset = params->args[2]" is the implicit conversion fails. Can
> that failure be recovered later? If not (and we are assigning something to offset just to keep
> compiling and produce as much diagnostics as we can), then I'd rather assign NULL to avoid further
> problems. Am I missing something?
>
> Thanks, Giovanni.

Hi,

Now that I look at it it doesn't make much sense,
perhaps I did it because the same thing was done for the coordinates
a couple of lines up:

if (!(coords = add_implicit_conversion(ctx, instrs, params->args[1],
        hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc)))
    coords = params->args[1];

Maybe it just makes sense to return false in both places, 
unless Zebediah has a reason to keep it this way.

[Zebediah Figura (she/her)]

On 12/20/21 06:51, Francisco Casas wrote:
> December 20, 2021 8:02 AM, "Giovanni Mascellani"  wrote:
> > Hi,
> >
> > On 17/12/21 20:12, Francisco Casas wrote:
> > > @@ -1917,9 +1916,16 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs,
> > > struct hl
> > > hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc)))
> > > coords = params->args[1];
> > > > - if (params->args_count == 3)
> > > - {
> > > - if (!(offset = add_implicit_conversion(ctx, instrs, params->args[2],
> > > - hlsl_get_vector_type(ctx, HLSL_TYPE_INT, sampler_dim), loc)))
> > > - offset = params->args[2];
> > > - }
> > > -
> >
> > I don't understand the point of "offset = params->args[2]" is the implicit conversion fails. Can
> > that failure be recovered later? If not (and we are assigning something to offset just to keep
> > compiling and produce as much diagnostics as we can), then I'd rather assign NULL to avoid further
> > problems. Am I missing something?
> >
> > Thanks, Giovanni.
>
> Hi,
>
> Now that I look at it it doesn't make much sense,
> perhaps I did it because the same thing was done for the coordinates
> a couple of lines up:
>
> if (!(coords = add_implicit_conversion(ctx, instrs, params->args[1],
>          hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc)))
>      coords = params->args[1];
>
> Maybe it just makes sense to return false in both places,
> unless Zebediah has a reason to keep it this way.

Yes, the idea was to allow compilation to proceed. It doesn't seem 
actually necessary for that to not be NULL here though; possibly it was 
the case in an earlier local version of the patch implementing Load(), 
or it was just a mistake on my part. So that can be removed.