[Piglit] [PATCH 2/4] arb_shader_precision: add framework for calculating tolerances for complex functions

[Ilia Mirkin]

On Thu, Feb 19, 2015 at 1:06 AM, Micah Fedke
<micah.fedke at collabora.co.uk> wrote:
> +def _gen_tolerance(name, rettype, args):
> +    """Return the tolerance that should be allowed for a function for the
> +    test vector passed in.  Return -1 for any vectors that would push the
> +    tolerance outside of acceptable bounds
> +    """
> +    if name in simple_fns:
> +        if name == 'op-mult' or name == 'op-assign-mult':

Seems like this should be outside of the if. So like

if name == 'op-mult' or name == ...:
elif name in simple_fns:
elif name in compelx_fns:

> +            x_type = glsl_type_of(args[0])
> +            y_type = glsl_type_of(args[1])
> +            if x_type.is_vector and y_type.is_matrix:
> +                mult_func = _vec_times_mat_ref
> +            elif x_type.is_matrix and y_type.is_vector:
> +                mult_func = _mat_times_vec_ref
> +            elif x_type.is_matrix and y_type.is_matrix:
> +                mult_func = _mat_times_mat_ref
> +            else:
> +                return simple_fns[name]
> +            ret = _analyze_ref_fn(mult_func, args)
> +            return -1.0 if any(ret['badlands']) else map(float,
> ret['component_tolerances'])
> +        else:
> +            return simple_fns[name]
> +    elif name in complex_fns:
> +        if name in componentwise_fns:
> +            ret = {'errors':[], 'badlands':[], 'component_tolerances':[]}

Is there some sort of advantage to keeping these in a dict?

> +            for component in range(rettype.num_cols*rettype.num_rows):

xrange. never use range (except when you really just need the array,
which is... never. or python3, which this isn't).

> +                current_args = []
> +                for i, arg in enumerate(args):
> +                    current_args.append(arg[component%len(arg)] if
> _len_any(arg) > 1 else arg)
> +                rettmp = _analyze_ref_fn(complex_fns[name], current_args)
> +                ret['errors'].extend(rettmp['errors'])
> +                ret['badlands'].extend(rettmp['badlands'])
> + ret['component_tolerances'].extend(rettmp['component_tolerances'])
> +        else:
> +            ret = _analyze_ref_fn(complex_fns[name], args)
> +        return -1.0 if any(ret['badlands']) else map(float,
> ret['component_tolerances'])
> +    else:
> +        return 0.0

This function is confusing. It sometimes returns floats, other times
arrays. What all these functions are doing is somewhat clever... can
you better describe the arguments and return values for these helpers
(especially this one)? And also comment things more? This is an
*extremely* dense function, I think it does a number of non-obvious
things. Would be nice to have comments explaining what you're trying
to account for or what some case is trying to handle.

Also, not sure if Dylan addressed this, but this is also unusual
spacing. a * b, not a*b, etc. This is how all the C code is written,
should carry over into Python as well.