On Wed Jan 25 12:34:56 2023 +0000, Giovanni Mascellani wrote:
This really deserves an explanation in the code.
Yes, a comment is not a bad idea here.
Note also that what makes this work is not so much that the load src
is uniform (and hence "real") but that it's constant. In fact, more broadly, we can safely copy-prop an object load *if* the load var doesn't change between the prior load and this resource instruction. We don't have the infrastructure right now to do that with all variables (we could if we wanted to, but it's probably not worthwhile—doing it with uniforms alone should probably be enough?) I think I agree, but this feels more subtle (for example, you have to be sure of what happens when you have a constant object initialized to a mutable one, which is later mutated, and be sure that the right thing happens). So I would avoid it until we have an indication that is required for feature parity with native. OTOH, I think that the current implementation is still somewhat problematic. I couldn't write an actual failing example, but I am concerned by something like:
a = b; b = c; d = a;Here `d` is supposed to be equal to the uniform `b`, but if you skip the first load of `b` during the first pass you end up pointing to the uniform `c`, in basically the same way as Zeb's example. I don't think this MR fixed that, because it is only concerned with object loads (and there there aren't). So either you do the check also for "regular" loads, or (better, I think) you don't even store a non-uniform object in the copy propagation state, with something like this patch (on top of this MR):
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index fa96ca4b..bab17c21 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -927,12 +927,6 @@ static bool copy_propagation_transform_object_load(struct hlsl_ctx *ctx, /* Only HLSL_IR_LOAD can produce an object. */ load = hlsl_ir_load(value->node); - if (!load->src.var->is_uniform) - { - TRACE("Ignoring load from non-uniform object variable %s\n", load->src.var->name); - return false; - } - hlsl_cleanup_deref(deref); hlsl_copy_deref(ctx, deref, &load->src); @@ -975,7 +969,20 @@ static void copy_propagation_record_store(struct hlsl_ctx *ctx, struct hlsl_ir_s unsigned int writemask = store->writemask; if (store->rhs.node->data_type->type == HLSL_CLASS_OBJECT) + { + struct hlsl_ir_load *load; + writemask = VKD3DSP_WRITEMASK_0; + + /* Only HLSL_IR_LOAD can produce an object. */ + load = hlsl_ir_load(store->rhs.node); + if (!load->src.var->is_uniform) + { + TRACE("Ignoring store of non-uniform object variable %s\n", load->src.var->name); + copy_propagation_invalidate_variable_from_deref(ctx, var_def, lhs, store->writemask); + return; + } + } copy_propagation_set_value(var_def, start, writemask, store->rhs.node); } else
I don't think the problem is currently happening with regular stores/loads, because when we do a replacement, we are removing the whole instruction (and replacing all subsequent references to it), so there is not the possibility of copy-prop doing an incorrect replacement for the instruction being replaced a second time.
Writing directly ``` a = b; b = c; d = a; ``` could be misleading, since on regular stores what we have on the rhs is an hlsl_src, a reference to another node, so it would be more accurate to write it as: ``` 1 : load (b) 2 : a = @1; 3 : load (c) 4 : b = @3; 5 : load (a) 6 : d = @5; ``` which would be transformed to ``` 1 : load (b) 2 : a = @1; 3 : load (c) 4 : b = @3; 5 : <deleted> 6 : d = @1; ```