[Mesa-dev] [PATCH] gallium: fixed modulo zero crashes in tgsi interpreter

Brian Paul brianp at vmware.com
Fri Jun 9 14:35:49 UTC 2017


Let's not impose behavior that would burden drivers (for example: "idiv 
by zero must result in ~0u").  Just updating the docs to say the results 
of div/mod by zero is undefined (unless we know something specific is 
needed) would be fine.  As it is now, some div/mod operations are 
documented to return 0 or ~0 but others say nothing and leaves the 
reader wondering what's expected.

-Brian

On 06/09/2017 03:22 AM, Marius Gräfe wrote:
> I can fix the remaining integer div/mod opcodes, no problem. I think a
> consistent error value would be beneficial, would opt for ~0u for 32-bit
> values and ~0ull for 64 bit values to keep it consistent with the only
> existing requirement (that is d3d10). Should I just submit another patch
> based on my original one with the fixed values? I am unsure about the
> correct procedure, still new to this whole mailing list procedure.
> As far as src/gallium/docs/source/tgsi.rst is concerned, the docs don't
> seem to mention any error value anyway.
>
> Marius
>
>
> Am 08.06.2017 um 22:28 schrieb Roland Scheidegger:
>> The behavior is probably undefined for most of the opcodes (signed 32bit
>> div, all 64bit div/mod), the docs don't state anything. But in general,
>> this is all undefined in all apis (opencl, glsl, spir-v), with the only
>> exception being d3d10 - which only has udiv and umod, hence these
>> stating in the gallium docs the required result (~0u).
>> So, I suppose we could let the docs say it's actually undefined, right
>> now gallivm will actually return 0 for idiv but all-ones for idiv64 and
>> so on, it's not really consistent...
>>
>> Roland
>>
>>
>> Am 08.06.2017 um 21:51 schrieb Brian Paul:
>>> Marius,
>>>
>>> As long as you're working on this, would you review
>>> src/gallium/docs/source/tgsi.rst to check if all the div/mod
>>> instructions document div/mod by zero behavior?  Thanks.
>>>
>>> -Brian
>>>
>>>
>>> On 06/08/2017 11:10 AM, Roland Scheidegger wrote:
>>>> I don't really know if it makes sense to have different "error values"
>>>> for signed vs. unsigned modulo 0 - maybe the "all bits set" approach
>>>> would do too (the gallivm code does this, because it is actually
>>>> easier). But since it's undefined in any case pretty much everywhere, I
>>>> suppose any value will do (only the 32bit umod really has a requirement
>>>> for all bits set due do d3d10 requirements, but d3d has neither signed
>>>> nor 64bit versions of it).
>>>> And I don't know why it would only crash in geometry shaders...
>>>>
>>>> Reviewed-by: Roland Scheidegger <sroland at vmware.com>
>>>>
>>>>
>>>> Am 08.06.2017 um 18:28 schrieb Marius Gräfe:
>>>>> softpipe throws integer division by zero exceptions on windows
>>>>> when using % with integers in a geometry shader.
>>>>> ---
>>>>>    src/gallium/auxiliary/tgsi/tgsi_exec.c | 24
>>>>> ++++++++++++------------
>>>>>    1 file changed, 12 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c
>>>>> b/src/gallium/auxiliary/tgsi/tgsi_exec.c
>>>>> index c41954c..abd2d16 100644
>>>>> --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c
>>>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c
>>>>> @@ -866,20 +866,20 @@ static void
>>>>>    micro_u64mod(union tgsi_double_channel *dst,
>>>>>                 const union tgsi_double_channel *src)
>>>>>    {
>>>>> -   dst->u64[0] = src[0].u64[0] % src[1].u64[0];
>>>>> -   dst->u64[1] = src[0].u64[1] % src[1].u64[1];
>>>>> -   dst->u64[2] = src[0].u64[2] % src[1].u64[2];
>>>>> -   dst->u64[3] = src[0].u64[3] % src[1].u64[3];
>>>>> +   dst->u64[0] = src[1].u64[0] ? src[0].u64[0] % src[1].u64[0] :
>>>>> UINT64_MAX;
>>>>> +   dst->u64[1] = src[1].u64[1] ? src[0].u64[1] % src[1].u64[1] :
>>>>> UINT64_MAX;
>>>>> +   dst->u64[2] = src[1].u64[2] ? src[0].u64[2] % src[1].u64[2] :
>>>>> UINT64_MAX;
>>>>> +   dst->u64[3] = src[1].u64[3] ? src[0].u64[3] % src[1].u64[3] :
>>>>> UINT64_MAX;
>>>>>    }
>>>>>
>>>>>    static void
>>>>>    micro_i64mod(union tgsi_double_channel *dst,
>>>>>                 const union tgsi_double_channel *src)
>>>>>    {
>>>>> -   dst->i64[0] = src[0].i64[0] % src[1].i64[0];
>>>>> -   dst->i64[1] = src[0].i64[1] % src[1].i64[1];
>>>>> -   dst->i64[2] = src[0].i64[2] % src[1].i64[2];
>>>>> -   dst->i64[3] = src[0].i64[3] % src[1].i64[3];
>>>>> +   dst->i64[0] = src[1].i64[0] ? src[0].i64[0] % src[1].i64[0] :
>>>>> INT64_MAX;
>>>>> +   dst->i64[1] = src[1].i64[1] ? src[0].i64[1] % src[1].i64[1] :
>>>>> INT64_MAX;
>>>>> +   dst->i64[2] = src[1].i64[2] ? src[0].i64[2] % src[1].i64[2] :
>>>>> INT64_MAX;
>>>>> +   dst->i64[3] = src[1].i64[3] ? src[0].i64[3] % src[1].i64[3] :
>>>>> INT64_MAX;
>>>>>    }
>>>>>
>>>>>    static void
>>>>> @@ -4653,10 +4653,10 @@ micro_mod(union tgsi_exec_channel *dst,
>>>>>              const union tgsi_exec_channel *src0,
>>>>>              const union tgsi_exec_channel *src1)
>>>>>    {
>>>>> -   dst->i[0] = src0->i[0] % src1->i[0];
>>>>> -   dst->i[1] = src0->i[1] % src1->i[1];
>>>>> -   dst->i[2] = src0->i[2] % src1->i[2];
>>>>> -   dst->i[3] = src0->i[3] % src1->i[3];
>>>>> +   dst->i[0] = src1->i[0] ? src0->i[0] % src1->i[0] : INT_MAX;
>>>>> +   dst->i[1] = src1->i[1] ? src0->i[1] % src1->i[1] : INT_MAX;
>>>>> +   dst->i[2] = src1->i[2] ? src0->i[2] % src1->i[2] : INT_MAX;
>>>>> +   dst->i[3] = src1->i[3] ? src0->i[3] % src1->i[3] : INT_MAX;
>>>>>    }
>>>>>
>>>>>    static void
>>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIDaQ&c=uilaK90D4TOVoH58JNXRgQ&r=Ie7_encNUsqxbSRbqbNgofw0ITcfE8JKfaUjIQhncGA&m=q8rVSBYk-twDGIUddzifD3ycV0iJmBFApCqEqWLcwRQ&s=xynE8kAQ9Nbg0DjqTeKdapgkJWfDpA2CBeTxhcdOvIM&e=
>>>>
>



More information about the mesa-dev mailing list