[Mesa-dev] [PATCH 3/6] i965/fs: Do a general SEND dependency workaround for the original 965.

[Eric Anholt]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On 02/06/2013 05:29 PM, Eric Anholt wrote:
>> We'd been ad-hoc inserting instructions in some SEND messages with no
>> knowledge of when it was required (so extra instructions), but not all SENDs
>> (so not often enough).  This should do much better than that, though it's
>> still flow-control-ignorant.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=58960
>> NOTE: Candidate for the stable branches.
>> ---
>>   src/mesa/drivers/dri/i965/brw_fs.cpp      |  225 +++++++++++++++++++++++++++++
>>   src/mesa/drivers/dri/i965/brw_fs.h        |    4 +
>>   src/mesa/drivers/dri/i965/brw_fs_emit.cpp |   42 ------
>>   3 files changed, 229 insertions(+), 42 deletions(-)
>
> Presumably we should have one of these for the vertex shader as well. :(

Well, rlen in the VS is usually 1, and it'll get consumed, so it's much
less of a problem there.  But yes.

> Perhaps:
> bool needs_dep[BRW_MAX_MRF];
>
> I guess it's fine, though...even if someday we expand BRW_MAX_MRF to 
> more than 16 (for Sandybridge), this workaround only applies to Gen4 
> which will always have exactly 16.

Will do.

>> +   assert(write_len < (int)sizeof(needs_dep) - 1);
>> +
>> +   memset(needs_dep, false, sizeof(needs_dep));
>> +   memset(needs_dep, true, write_len);
>
> The second memset only works if sizeof(bool) == 1.  While that's likely 
> true with our toolchain, it's explicitly _not_ guaranteed.
>
>  From the C++98 specification, section 5.3.3 Sizeof [expr.sizeof], 
> paragraph 1: "in particular, sizeof(bool) and sizeof(wchar_t) are 
> implementation-defined." Footnote 65 reiterates: "sizeof(bool) is not 
> required to be 1."
>
> I'd be more comfortable with:
>
>      memset(needs_dep, true, write_len * sizeof(bool));
> or the obvious loop.

Well, language specs also let ints be other than 32-bit (and actual
compilers do have non-32-bit ints), but we haven't let that stop us.

But more seriously, I can only see 2 other sizes that bool might be:
either a single bit (in which case memset() won't work at all), or int,
in which case we'll be memsetting the bool to 0x01010101.  Neither of
those would be valid, and you'd have to turn all nonzero bool
initialization into manual loops.  Besides, if you had a compiler making
over 8-bit bools, we'd want to figure out how to fix that since it would
totally blow out caches in the register allocator.

I'd rather leave things as-is in this case.
>> +void
>> +fs_visitor::insert_gen4_send_dependency_workarounds()
>> +{
>> +   if (intel->gen != 4 || intel->is_g4x)
>> +      return;
>
> Is this actually necessary for g4x?

This is "if you're g4x, return" :)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130214/2f2b17f9/attachment-0001.pgp>