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

[Micah Fedke]

On 02/20/2015 05:31 PM, Ilia Mirkin wrote:
> 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:
>

It could work just as well that way, yes.  My intent was to first split 
the names into two mutually exclusive high-level groups (simple_fns and 
complex_fns), and then treat individual members of those groups as 
necessary (eg. the mult operations, which only appear within 
simple_fns).  I'd like to leave it this way for clarity, if you agree? 
I can add a comment line somewhere about the mutual exclusivity of the 
two groups.

>> +            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?

The dict is for clarity purposes only.  I guess I could use a named 
tuple instead?  It seemed like more code . . .

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

-- 

Micah Fedke
Collabora Ltd.
+44 1223 362967
https://www.collabora.com/
https://twitter.com/collaboraltd