[Mesa-dev] [PATCH] glsl: Fix type error when lowering integer divisions

[Paul Berry]

On 13 August 2011 09:58, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 08/12/2011 10:38 AM, Paul Berry wrote:
>>
>> This patch fixes a bug when lowering an integer division:
>>
>>   x/y
>>
>> to a multiplication by a reciprocal:
>>
>>   int(float(x)*reciprocal(float(y)))
>>
>> If x was a a plain int and y was an ivecN, the lowering pass
>> incorrectly assigned the type of the product to be float, when in fact
>> it should be vecN.  This caused mesa to abort with an IR validation
>> error.
>>
>> Fixes piglit tests {fs,vs}-op-div-int-ivec{2,3,4}.
>
> Good catch, Paul!  Thanks again for writing all these test cases.
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> Come to think of it, we may want to avoid this altogether on i965.  The
> mathbox has an INT DIV message that computes integer quotient and
> remainder...so we can support it natively.
>
> I guess the question is "which is faster?".  My intuition says that using
> INT DIV will be faster on Gen6+, possibly on Gen5, and slower on Gen4/G45.
>  AFAICT on Gen5+ you can compute quotient & remainder separately (3 or 4
> rounds) while on Gen4 you always have to compute both (3 + 4 = 7 rounds?).
>  Meanwhile RCP is 2 rounds.  Not only is that more rounds, it means hogging
> the shared mathbox for longer.

Accuracy is also a question.  Our current technique of multiplying by
the reciprocal doesn't work for some denominators because of rounding
errors in computing the reciprocal.  For example, try to write a
piglit tests that computes 77/77.  On Gen5 hardware, at least, this
produces zero.  The reason is because rounding errors in computing the
floating point reciprocal mean that  77*reciprocal(77) is actually
slightly less than 1.0, so it gets rounded down to zero when it's
converted back to an int.  Note: I believe the smallest integer for
which rounding errors cause n*reciprocal(n) to be less than 1.0 is
n=25, which probably explains why we haven't noticed this bug before.

As Eric pointed out, this situation is clearly unacceptable for GLSL
1.30 (which requires ints to truly have 32 bits of precision).  It's
less clear to me what's ok in previous versions of GLSL.  The spec
says we may approximate integer operations using floats, so if you
take them at their word, what we're doing is ok.  But I'm guessing the
reason they said that was ok was because they expected floating point
operations on small integers to yield perfectly precise results (and
they do, provided you don't do the trick of computing x/y using
x*reciprocal(y)).  So I would argue that we're keeping the word of the
spec but violating its spirit: people are really going to expect that
n/n=1 for all n.

My personal feeling: even if there's a penalty in performance, let's
go ahead and implement true integer division on as many platforms as
we can without a large amount of development effort, even if those
platforms are never going to support GLSL 1.30.  But let's also stick
with this bug fix for the benefit of platforms where true integer
division is impractical (e.g. because it would require too much driver
rework, or because the chipset lacks an integer divide instruction).