From: Zebediah Figura Subject: Re: [PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types. Message-Id: <9ace044b-dd73-b087-8320-939537441747@codeweavers.com> Date: Fri, 26 Jun 2020 12:14:12 -0500 In-Reply-To: References: <20200622224722.135396-1-zfigura@codeweavers.com> <20200622224722.135396-5-zfigura@codeweavers.com> <6e298b8a-8ede-42ed-e18d-e14d4c2d2c49@codeweavers.com> On 6/26/20 11:28 AM, Matteo Bruni wrote: > On Fri, Jun 26, 2020 at 6:02 PM Zebediah Figura wrote: >> >> On 6/26/20 10:56 AM, Matteo Bruni wrote: >>> On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura wrote: >>>> >>>> Signed-off-by: Zebediah Figura >>>> --- >>>> dlls/d3dcompiler_43/hlsl.y | 19 +++++++++++++++---- >>>> dlls/d3dcompiler_43/utils.c | 11 ++++++++--- >>>> 2 files changed, 23 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y >>>> index 21828337939..27aab195b25 100644 >>>> --- a/dlls/d3dcompiler_43/hlsl.y >>>> +++ b/dlls/d3dcompiler_43/hlsl.y >>>> @@ -416,11 +416,17 @@ static struct hlsl_ir_swizzle *new_swizzle(DWORD s, unsigned int components, >>>> struct hlsl_ir_node *val, struct source_location *loc) >>>> { >>>> struct hlsl_ir_swizzle *swizzle = d3dcompiler_alloc(sizeof(*swizzle)); >>>> + struct hlsl_type *data_type; >>>> >>>> if (!swizzle) >>>> return NULL; >>>> - init_node(&swizzle->node, HLSL_IR_SWIZZLE, >>>> - new_hlsl_type(NULL, HLSL_CLASS_VECTOR, val->data_type->base_type, components, 1), *loc); >>>> + >>>> + if (components == 1) >>>> + data_type = hlsl_ctx.builtin_types.scalar[val->data_type->base_type]; >>>> + else >>>> + data_type = hlsl_ctx.builtin_types.vector[val->data_type->base_type][components - 1]; >>>> + >>>> + init_node(&swizzle->node, HLSL_IR_SWIZZLE, data_type, *loc); >>>> swizzle->val = val; >>>> swizzle->swizzle = s; >>>> return swizzle; >>>> @@ -2488,6 +2494,7 @@ postfix_expr: primary_expr >>>> for (i = 0; i < $4.args_count; ++i) >>>> { >>>> struct hlsl_ir_node *arg = $4.args[i]; >>>> + struct hlsl_type *data_type; >>>> unsigned int width; >>>> >>>> if (arg->data_type->type == HLSL_CLASS_OBJECT) >>>> @@ -2504,8 +2511,12 @@ postfix_expr: primary_expr >>>> continue; >>>> } >>>> >>>> - if (!(arg = add_implicit_conversion($4.instrs, arg, >>>> - hlsl_ctx.builtin_types.vector[$2->base_type][width - 1], &arg->loc))) >>>> + if (width == 1) >>>> + data_type = hlsl_ctx.builtin_types.scalar[$2->base_type]; >>>> + else >>>> + data_type = hlsl_ctx.builtin_types.vector[$2->base_type][width - 1]; >>>> + >>>> + if (!(arg = add_implicit_conversion($4.instrs, arg, data_type, &arg->loc))) >>>> continue; >>>> >>>> if (!(assignment = new_assignment(var, NULL, arg, >>>> diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c >>>> index 30aa9de1dd4..4f0556103ab 100644 >>>> --- a/dlls/d3dcompiler_43/utils.c >>>> +++ b/dlls/d3dcompiler_43/utils.c >>>> @@ -1294,7 +1294,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type >>>> } >>>> } >>>> >>>> - if (type == HLSL_CLASS_SCALAR) >>>> + if (type == HLSL_CLASS_SCALAR || (type == HLSL_CLASS_VECTOR && dimx == 1)) >>>> return hlsl_ctx.builtin_types.scalar[base]; >>>> if (type == HLSL_CLASS_VECTOR) >>>> return hlsl_ctx.builtin_types.vector[base][dimx - 1]; >>>> @@ -1496,9 +1496,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh >>>> d3dcompiler_free(assign); >>>> return NULL; >>>> } >>>> - assert(swizzle_type->type == HLSL_CLASS_VECTOR); >>>> + assert(swizzle_type->type == HLSL_CLASS_VECTOR || swizzle_type->type == HLSL_CLASS_SCALAR); >>>> if (swizzle_type->dimx != width) >>>> - swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1]; >>>> + { >>>> + if (width == 1) >>>> + swizzle->node.data_type = hlsl_ctx.builtin_types.scalar[swizzle_type->base_type]; >>>> + else >>>> + swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1]; >>>> + } >>>> rhs = &swizzle->node; >>>> } >>>> else >>> >>> What do we gain with this? >>> >> >> Not having to deal with vec1 at codegen time, basically, or along >> similar lines not having superfluous instructions to cast it to scalar. >> Granted, it still exists, but you'd have to use it intentionally... > > My main objection is that, IIRC, we need to keep the distinction > anyway for uniform variables (e.g. I guess it's visible in the > constant table). At that point we could just keep it like that all the > way through the compiler. > WRT casts, they can be avoided by figuring out at "codegen" that they > are actually the same type. Again, we probably need something along > those lines anyway since, especially in SM1, we have to map pretty > much everything to float eventually. > > Notice that I can still be convinced that this patch goes in the right > direction... > It gets a little tricky, I think. Uniforms are one case and pretty clear-cut, i.e. you declare something as "float" or "float1". Arbitrary expressions are another. The language *does* seem to care about these in at least one way I could find: you can index a vector using [0], but not a scalar. According to this criterion, both swizzles and expression argument coercion count as scalars rather than vectors. I.e. the code uniform float1 v; return v.x[0]; and uniform float1 v; uniform float s; return (v + s)[0]; will both throw an error. The third case in the patch above, of course, can't be tested, as it only has internal effect. Certainly I agree that nothing after parse time should care about the difference between float and float1, though. I guess we want to reduce any such generated casts using a copy-prop pass. -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEENTo75Twe9rbPART3DZ01igeheEAFAl72LOUACgkQDZ01igeh eEDKPwf+IrO665K0jpMng1j6zM2Hn0+HaLo+y9ZUgq1jFTnjea/umrNz9oHWbLhd 0oQDyX/tSy+dY/zcLCB6K0K8Qaxib3H4X0dK8sDT9mpZNjwphPQZv0VMdIB6PKjj mU3m2aBmF18lcYt+LAosEJxNEmVAu8skKUSEHyE7fohDHth2dzZTwjZ2qH0U0uxp duI/m0+Lnykj6ccIcEe00a+J5a+od6am3n6mPE/dye6CZDHePyOIk2yUf3bdAHGo rs58AO8jFCLt+X3IqPL6d/lyZzM2JlZsycuj3FH9uzLPCfAJakQl21ygz7eYWWSZ 5/oD8bHp95zJ3u2PTAIf9dr2Fi7qPA== =OjMO -----END PGP SIGNATURE-----