On Mon, 2016-08-01 at 10:01 +1000, Timothy Arceri wrote: > On Sun, 2016-07-31 at 18:43 +0300, Andres Gomez wrote:
[snip]
> diff --git a/src/compiler/glsl/ast_function.cpp
> > b/src/compiler/glsl/ast_function.cpp
> > index 9dcec50..9b09cb6 100644
> > --- a/src/compiler/glsl/ast_function.cpp
> > +++ b/src/compiler/glsl/ast_function.cpp
> > @@ -31,10 +31,6 @@
> > static ir_rvalue *
> > convert_component(ir_rvalue *src, const glsl_type *desired_type);
> >
> > -bool
> > -apply_implicit_conversion(const glsl_type *to, ir_rvalue * &from,
> > - struct _mesa_glsl_parse_state *state);
> > -
>
> I'd put this in a third patch when you remove this and
> make apply_implicit_conversion() static.
That makes sense. I would also remove the actual usage of
"apply_implicit_conversion" in "process_record_constructor" in that
patch too.
[snip]
> > @@ -1710,7 +1723,7 @@ process_record_constructor(exec_list
> > *instructions,
> >
> > exec_node *node = actual_parameters.get_head_raw();
> > for (unsigned i = 0; i < constructor_type->length; i++) {
> > - ir_rvalue *ir = (ir_rvalue *) node;
> > + ir_rvalue *result = (ir_rvalue *) node;
>
> Maybe its just me but I find this naming really confusing when trying
> to read the following code, I'd just leave it as ir.
I was doubting about this also. I decided to change the name for
coherence with other chunks of code in this file which perform similar
checks and conversions. See "process_vec_mat_constructor" and
"process_array_constructor".
Do you still prefer to leave it as "ir"?
> >
> > if (node->is_tail_sentinel()) {
> > _mesa_glsl_error(loc, state,
> > @@ -1719,18 +1732,35 @@ process_record_constructor(exec_list
> > *instructions,
> > return ir_rvalue::error_value(ctx);
> > }
> >
> > - if (apply_implicit_conversion(constructor_type-
> > > fields.structure[i].type,
> > - ir, state)) {
> > - node->replace_with(ir);
> > - } else {
> > - _mesa_glsl_error(loc, state,
> > - "parameter type mismatch in constructor
> > for `%s.%s' "
> > - "(%s vs %s)",
> > - constructor_type->name,
> > - constructor_type-
> > > fields.structure[i].name,
> > - ir->type->name,
> > - constructor_type-
> > > fields.structure[i].type->name);
> > + const glsl_base_type element_base_type =
> > + constructor_type->fields.structure[i].type->base_type;
> > +
> > + /* Apply implicit conversions (not the scalar constructor
> > rules!). See
> > + * the spec quote above. */
>
> */ should be on the next line
Right! I will update.
> > + if (element_base_type != result->type->base_type) {
> > + const glsl_type *desired_type =
> > + glsl_type::get_instance(element_base_type,
> > + result->type->vector_elements,
> > + result->type->matrix_columns);
> > +
> > + if (result->type->can_implicitly_convert_to(desired_type,
> > state)) {
> > + /* Even though convert_component() implements the
> > constructor
> > + * conversion rules (not the implicit conversion rules),
> > its safe
> > + * to use it here because we already checked that the
> > implicit
> > + * conversion is legal.
> > + */
> > + result = convert_component(result, desired_type);
> > + }
>
> You are adding a bunch of tabs above please remove them. It would be
> nice if you could add a forth patch to the series that removes the
> remain tabs in this file since there are not many remaining.
Ouch! The curse of copy and paste ☹
I will update and add that 4th patch.
Now that you comment that, maybe you can give a review to?
https://patchwork.freedesktop.org/patch/97838/
> > + }
> > +
> > + if (result->type != constructor_type-
> > > fields.structure[i].type) {
> > + _mesa_glsl_error(loc, state, "type error in array
> > constructor: "
> > + "expected: %s, found %s",
> > + constructor_type-
> > > fields.structure[i].type->name,
> > + result->type->name);
> > return ir_rvalue::error_value(ctx);
> > + } else {
> > + node->replace_with(result);
> > }
>
> This if-else should be inside the previous if.
>
> The node->replace_with(result); should be after
> result = convert_component(result, desired_type);
>
> Otherwise you are replacing the node with itself.
>
> The error message can be added as an else to if (result->type-
> > can_implicitly_convert_to())
Argh! Yeah, that's a shame.
I will update.
> >
> > node = node->next;
>
> I'm still not entirely sure I understand why this patch is different
> from using apply_implicit_conversion() can you explain to me why
> apply_implicit_conversion() does not match the rules correctly? Thanks
> :)
"apply_implicit_conversion" in that code was added at:
> commit f32d3df8ab2b7c6c746f46870edc4b284cea50ca
> Author: Kenneth Graunke <kenn...@whitecape.org>
> Date: Wed Sep 1 20:03:17 2010 -0700
>
> glsl: Apply implicit conversions to structure constructor parameters.
>
> The code for handling implicit conversions should probably get
> refactored, but for now, this is easy.
>
> Fixes piglit test constructor-26.vert.
As the commit states, it was a quick fix to make that piglit test pass.
The problem is that "apply_implicit_conversion" doesn't strictly check
for the rules in Section 4.1.10 “Implicit Conversions.” Specifically,
it allows conversions from a vector or a bigger dimension to a vector
or a smaller dimension. That was making the CTS test fail.
Now, the actual fix comes from using
"glsl_type::can_implicitly_convert_to" to check for those conversion
rules. We could still keep using "apply_implicit_conversion" but that
it is not necessary any more. Instead, we can replace with the local
"convert_component" and make the code more homogeneous and coherent
with the rest of the code in this file.
I hope that explains the changes. In any case, fragmenting in 3 patches
will make this more evident.
FTR, I run a complete pass on the piglit and CTS tests and couldn't
detect any regression with this series.
Thanks for the review!
--
Br,
Andres
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
