On Mon, 2016-08-01 at 12:05 +0300, Andres Gomez wrote:
> 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.
I'd leave the remove of the usage in this patch otherwise it won't make
much sense.
>
> [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"?
Up to you. I don't like it but I don't care enough to make you change
it if you think it makes more sense :)
>
> > >
> > > 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 see thanks. In that case I would suggest you make a new helper in a
patch before this one with the below code and make use of it with the
other two functions rather than copy and pasting it everywhere.
const glsl_type *desired_type =
glsl_type::get_instance(element_base_type,
ir->type->vector_elements,
ir->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(ir, desired_type);
}
>
> 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!
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
