[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Matteo Bruni]

On Fri, Jun 26, 2020 at 7:14 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
>
> On 6/26/20 11:28 AM, Matteo Bruni wrote:
> > On Fri, Jun 26, 2020 at 6:02 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
> >>
> >> On 6/26/20 10:56 AM, Matteo Bruni wrote:
> >>> On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
> >>>>
> >>>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> >>>> ---
> >>>>  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.

Oh, that's interesting. So the relevant part of this patch is
technically required for correctness.

> 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.

Right. My feeling is that such a pass simplifying redundant casts
effectively covers all other cases but I certainly haven't thought it
through.