[Mesa-stable] [PATCH] gallivm: make sampling more robust against bogus coordinates
Roland Scheidegger
sroland at vmware.com
Mon Apr 25 14:52:36 UTC 2016
Am 25.04.2016 um 15:44 schrieb Jose Fonseca:
> Looks good AFAICT. Can there be an impact in performance?
Yes, but I think it should be fairly minimal for the affected modes - at
least int compare / min instructions are cheap (too bad we only have
unsigned min with/max with sse41 but not compare).
mirror_repeat (using coord_mirror) actually might not be optimal -
probably could use unsigned min/max later there instead of signed ones
and skip the extra float min. But it's always a question what the cpu
actually supports - if there's only sse2 integer min/max have to be
emulated (and since there's no unsigned comparisons, emulation gets more
complex in this case), float min/max not.
>
> Reviewed-by: Jose Fonseca <jfonseca at vmware.com>
>
> I also think we should have piglit testcases for these things. Would it
> be easy to modify one of the existing textrwap to use Nans? We need to
> use BGRA8 and something else (e.g., RGBA32F) to covert both AoS/SoA paths.
Sounds like a good idea (I suppose though it's really only interesting
for sw based drivers).
Roland
>
> Jose
>
>
>
> On 22/04/16 14:33, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> Some cases (especially these using fract for coord wrapping) did not
>> handle
>> NaNs (or Infs) correctly - the following code assumed the fract result
>> could not be outside [0,1], but if the input is a NaN (or +-Inf) the
>> fract
>> result was NaN - which then could produce out-of-bound offsets.
>>
>> (Note that the explicit NaN behavior changes for min/max on x86 sse don't
>> result in actual changes in the generated jit code, but may on other
>> architectures. Found by looking through all the wrap functions.)
>>
>> This fixes https://bugs.freedesktop.org/show_bug.cgi?id=94955
>>
>> Cc: "11.1 11.2" <mesa-stable at lists.freedesktop.org>
>> ---
>> src/gallium/auxiliary/gallivm/lp_bld_arit.c | 9 ++++---
>> src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c | 13 ++++++++-
>> src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c | 33
>> +++++++++++++++++------
>> 3 files changed, 42 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
>> b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
>> index beff414..17cf296 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
>> @@ -2069,8 +2069,8 @@ lp_build_fract(struct lp_build_context *bld,
>>
>>
>> /**
>> - * Prevent returning a fractional part of 1.0 for very small negative
>> values of
>> - * 'a' by clamping against 0.99999(9).
>> + * Prevent returning 1.0 for very small negative values of 'a' by
>> clamping
>> + * against 0.99999(9). (Will also return that value for NaNs.)
>> */
>> static inline LLVMValueRef
>> clamp_fract(struct lp_build_context *bld, LLVMValueRef fract)
>> @@ -2080,13 +2080,14 @@ clamp_fract(struct lp_build_context *bld,
>> LLVMValueRef fract)
>> /* this is the largest number smaller than 1.0 representable as
>> float */
>> max = lp_build_const_vec(bld->gallivm, bld->type,
>> 1.0 - 1.0/(1LL <<
>> (lp_mantissa(bld->type) + 1)));
>> - return lp_build_min(bld, fract, max);
>> + return lp_build_min_ext(bld, fract, max,
>> + GALLIVM_NAN_RETURN_OTHER_SECOND_NONNAN);
>
>> }
>>
>>
>> /**
>> * Same as lp_build_fract, but guarantees that the result is always
>> smaller
>> - * than one.
>> + * than one. Will also return the smaller-than-one value for infs, NaNs.
>> */
>> LLVMValueRef
>> lp_build_fract_safe(struct lp_build_context *bld,
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
>> b/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
>> index 729c5b8..6bf92c8 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
>> @@ -246,6 +246,12 @@ lp_build_coord_repeat_npot_linear_int(struct
>> lp_build_sample_context *bld,
>> mask = lp_build_compare(int_coord_bld->gallivm, int_coord_bld->type,
>> PIPE_FUNC_LESS, *coord0_i,
>> int_coord_bld->zero);
>> *coord0_i = lp_build_select(int_coord_bld, mask,
>> length_minus_one, *coord0_i);
>> + /*
>> + * We should never get values too large - except if coord was nan
>> or inf,
>> + * in which case things go terribly wrong...
>> + * Alternatively, could use fract_safe above...
>> + */
>> + *coord0_i = lp_build_min(int_coord_bld, *coord0_i, length_minus_one);
>> }
>>
>>
>> @@ -490,6 +496,10 @@ lp_build_sample_wrap_linear_float(struct
>> lp_build_sample_context *bld,
>> *coord1 = lp_build_add(coord_bld, coord, half);
>> coord = lp_build_sub(coord_bld, coord, half);
>> *weight = lp_build_fract(coord_bld, coord);
>> + /*
>> + * It is important for this comparison to be unordered
>> + * (or need fract_safe above).
>> + */
>> mask = lp_build_compare(coord_bld->gallivm, coord_bld->type,
>> PIPE_FUNC_LESS, coord,
>> coord_bld->zero);
>> *coord0 = lp_build_select(coord_bld, mask,
>> length_minus_one, coord);
>> @@ -514,7 +524,8 @@ lp_build_sample_wrap_linear_float(struct
>> lp_build_sample_context *bld,
>> coord = lp_build_sub(coord_bld, coord, half);
>> }
>> /* clamp to [0, length - 1] */
>> - coord = lp_build_min(coord_bld, coord, length_minus_one);
>> + coord = lp_build_min_ext(coord_bld, coord, length_minus_one,
>> + GALLIVM_NAN_RETURN_OTHER_SECOND_NONNAN);
>> coord = lp_build_max(coord_bld, coord, coord_bld->zero);
>> *coord1 = lp_build_add(coord_bld, coord, coord_bld->one);
>> /* convert to int, compute lerp weight */
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> b/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> index 1727105..ace24fd 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> @@ -228,11 +228,15 @@ lp_build_coord_mirror(struct
>> lp_build_sample_context *bld,
>> LLVMValueRef fract, flr, isOdd;
>>
>> lp_build_ifloor_fract(coord_bld, coord, &flr, &fract);
>> + /* kill off NaNs */
>> + fract = lp_build_min_ext(coord_bld, fract, coord_bld->zero,
>> + GALLIVM_NAN_RETURN_OTHER_SECOND_NONNAN);
>>
>> /* isOdd = flr & 1 */
>> isOdd = LLVMBuildAnd(bld->gallivm->builder, flr,
>> int_coord_bld->one, "");
>>
>> /* make coord positive or negative depending on isOdd */
>> + /* XXX slight overkill masking out sign bit is unnecessary */
>> coord = lp_build_set_sign(coord_bld, fract, isOdd);
>>
>> /* convert isOdd to float */
>> @@ -272,10 +276,15 @@ lp_build_coord_repeat_npot_linear(struct
>> lp_build_sample_context *bld,
>> * we avoided the 0.5/length division before the repeat wrap,
>> * now need to fix up edge cases with selects
>> */
>> + /*
>> + * Note we do a float (unordered) compare so we can eliminate NaNs.
>> + * (Otherwise would need fract_safe above).
>> + */
>> + mask = lp_build_compare(coord_bld->gallivm, coord_bld->type,
>> + PIPE_FUNC_LESS, coord_f, coord_bld->zero);
>> +
>> /* convert to int, compute lerp weight */
>> lp_build_ifloor_fract(coord_bld, coord_f, coord0_i, weight_f);
>> - mask = lp_build_compare(int_coord_bld->gallivm, int_coord_bld->type,
>> - PIPE_FUNC_LESS, *coord0_i,
>> int_coord_bld->zero);
>> *coord0_i = lp_build_select(int_coord_bld, mask,
>> length_minus_one, *coord0_i);
>> }
>>
>> @@ -375,7 +384,8 @@ lp_build_sample_wrap_linear(struct
>> lp_build_sample_context *bld,
>> }
>>
>> /* clamp to length max */
>> - coord = lp_build_min(coord_bld, coord, length_f);
>> + coord = lp_build_min_ext(coord_bld, coord, length_f,
>> +
>> GALLIVM_NAN_RETURN_OTHER_SECOND_NONNAN);
>> /* subtract 0.5 */
>> coord = lp_build_sub(coord_bld, coord, half);
>> /* clamp to [0, length - 0.5] */
>> @@ -398,7 +408,7 @@ lp_build_sample_wrap_linear(struct
>> lp_build_sample_context *bld,
>> coord = lp_build_add(coord_bld, coord, offset);
>> }
>> /* was: clamp to [-0.5, length + 0.5], then sub 0.5 */
>> - /* can skip clamp (though might not work for very large coord
>> values */
>> + /* can skip clamp (though might not work for very large coord
>> values) */
>> coord = lp_build_sub(coord_bld, coord, half);
>> /* convert to int, compute lerp weight */
>> lp_build_ifloor_fract(coord_bld, coord, &coord0, &weight);
>> @@ -465,7 +475,8 @@ lp_build_sample_wrap_linear(struct
>> lp_build_sample_context *bld,
>> coord = lp_build_abs(coord_bld, coord);
>>
>> /* clamp to length max */
>> - coord = lp_build_min(coord_bld, coord, length_f);
>> + coord = lp_build_min_ext(coord_bld, coord, length_f,
>> +
>> GALLIVM_NAN_RETURN_OTHER_SECOND_NONNAN);
>> /* subtract 0.5 */
>> coord = lp_build_sub(coord_bld, coord, half);
>> /* clamp to [0, length - 0.5] */
>> @@ -628,9 +639,15 @@ lp_build_sample_wrap_nearest(struct
>> lp_build_sample_context *bld,
>>
>> /* itrunc == ifloor here */
>> icoord = lp_build_itrunc(coord_bld, coord);
>> -
>> - /* clamp to [0, length - 1] */
>> - icoord = lp_build_min(int_coord_bld, icoord, length_minus_one);
>> + /*
>> + * Use unsigned min due to possible undef values (NaNs, overflow)
>> + */
>> + {
>> + struct lp_build_context abs_coord_bld = *int_coord_bld;
>> + abs_coord_bld.type.sign = FALSE;
>> + /* clamp to [0, length - 1] */
>> + icoord = lp_build_min(&abs_coord_bld, icoord,
>> length_minus_one);
>> + }
>> break;
>>
>> case PIPE_TEX_WRAP_MIRROR_CLAMP_TO_BORDER:
>>
>
More information about the mesa-stable
mailing list