On 4/1/20 2:33 PM, Matteo Bruni wrote:
On Wed, Apr 1, 2020 at 9:00 PM Zebediah Figura zfigura@codeweavers.com wrote:
On 4/1/20 1:34 PM, Matteo Bruni wrote:
On Mon, Mar 30, 2020 at 4:54 AM Zebediah Figura z.figura12@gmail.com wrote:
From: Zebediah Figura zfigura@codeweavers.com
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
dlls/d3dcompiler_43/d3dcompiler_private.h | 3 +++ dlls/d3dcompiler_43/hlsl.y | 23 +++++++++++++++++++++++ dlls/d3dcompiler_43/utils.c | 9 +++++++++ 3 files changed, 35 insertions(+)
diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h index df45a7082fe..4fdb464a4ef 100644 --- a/dlls/d3dcompiler_43/d3dcompiler_private.h +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h @@ -614,6 +614,7 @@ struct hlsl_type unsigned int modifiers; unsigned int dimx; unsigned int dimy;
- unsigned int reg_size; union { struct list *elements;
I'm not too thrilled to have the size stored in struct hlsl_type. I guess it would be very awkward otherwise though...
If you think this is bad, wait until you see what I have next :D
(Namely, I have patches to access the hlsl_type structure from the "bwriter" layer, and also to store type offsets in the DXBC blob. Because otherwise writing structure types involves a lot of pointless duplication, both in terms of HLSL code and generated DXBC, and honestly widl does the same thing and it works. If it helps, we could rename it to something other than hlsl_type...)
Sure, using struct hlsl_type throughout the stack is not a problem. hlsl_type is a bit of a bad name regardless. The point about "it works for widl" isn't impressing me much though :D
WRT DXBC offsets, you could have a struct bwriter_type (or other name) that points to the relevant hlsl_type and has the additional stuff you need for generating bytecode. It should be a bit nicer than sticking everything in hlsl_type and hopefully not as painful as fully duplicating everything (which I agree isn't helpful). It depends how much bwriter-specific stuff you're going to need.
Sure, that's definitely better than doing a whole translation. I'm mostly inclined to say it's not so much prettier that it's worth the extra allocation, but that's just me.
I think the only thing I needed the bwriter layer to store is the type offset (including for sm4 reflection).
I guess the alternative here is to have something like "unsigned int hlsl_type_get_reg_size(const struct hlsl_type *type)". Not even that ugly, but obviously means more overhead, and I imagine avoiding that overhead is a worthwhile goal for a compiler...
Yeah, I guess storing the size in there is okay. I'll see how my body will react to what you have in store next :D
@@ -632,6 +633,7 @@ struct hlsl_struct_field const char *name; const char *semantic; DWORD modifiers;
unsigned int reg_offset; };
struct source_location
@@ -1083,6 +1085,7 @@ struct hlsl_type *new_hlsl_type(const char *name, enum hlsl_type_class type_clas struct hlsl_type *new_array_type(struct hlsl_type *basic_type, unsigned int array_size) DECLSPEC_HIDDEN; struct hlsl_type *clone_hlsl_type(struct hlsl_type *old) DECLSPEC_HIDDEN; struct hlsl_type *get_type(struct hlsl_scope *scope, const char *name, BOOL recursive) DECLSPEC_HIDDEN; +BOOL is_row_major(const struct hlsl_type *type) DECLSPEC_HIDDEN; BOOL find_function(const char *name) DECLSPEC_HIDDEN; unsigned int components_count_type(struct hlsl_type *type) DECLSPEC_HIDDEN; BOOL compare_hlsl_types(const struct hlsl_type *t1, const struct hlsl_type *t2) DECLSPEC_HIDDEN; diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y index 312949c5840..d6c64edcace 100644 --- a/dlls/d3dcompiler_43/hlsl.y +++ b/dlls/d3dcompiler_43/hlsl.y @@ -717,6 +717,15 @@ static BOOL add_struct_field(struct list *fields, struct hlsl_struct_field *fiel return TRUE; }
+BOOL is_row_major(const struct hlsl_type *type) +{
- if (type->modifiers & HLSL_MODIFIER_ROW_MAJOR)
return TRUE;- if (type->modifiers & HLSL_MODIFIER_COLUMN_MAJOR)
return FALSE;- return hlsl_ctx.matrix_majority == HLSL_ROW_MAJOR;
+}
- static struct hlsl_type *apply_type_modifiers(struct hlsl_type *type, DWORD *modifiers, struct source_location loc) { struct hlsl_type *new_type;
@@ -729,6 +738,9 @@ static struct hlsl_type *apply_type_modifiers(struct hlsl_type *type, DWORD *mod
new_type->modifiers = add_modifiers(new_type->modifiers, *modifiers, loc); *modifiers &= ~HLSL_TYPE_MODIFIERS_MASK;
- if (new_type->type == HLSL_CLASS_MATRIX)
new_type->reg_size = is_row_major(new_type) ? new_type->dimy : new_type->dimx; return new_type;I think we should have one of the "majority" modifiers always stored in the hlsl_type for matrices, since it's actually a significant distinction. Notice that the default matrix majority can be changed over the course of a shader via a #pragma.
The reason I don't particularly want to do this is constructions like:
typedef float3x3 matrix_t; typedef row_major matrix_t row_matrix_t;
I didn't realize that the default majority could change, though. Maybe we need to store the default majority as a separate field in hlsl_type.
I don't think it matters a lot if it's stored in a separate field or together with the other modifiers. This is a bit of a special case regardless, in that you need to override the existing majority (rather than complaining that the other majority is currently set) in some cases. Certainly fine to change things if it helps though.
And yeah, tests do have room for improvement...
Mostly the trick is that a subsequent declaration "typedef column_major row_matrix_t col_matrix_t" throws up an error.
Anyway, I'll add some extra tests.