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.