[Mesa-dev] [PATCH v2 1/3] i965/fs: Use the source type when looking for UD negations in copy prop

[Jason Ekstrand]

On Mon, Apr 6, 2015 at 4:50 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Fri, Apr 3, 2015 at 2:06 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> There can be problems with floats and conditional modifiers when
>> copy-propagating a negated UD source.  Previously, we checked the source to
>> be copied for the negate and then checked the source being propagated to for
>> the type.  This isn't quite what we want because we are really just looking
>> for negated UD sources.  A check later in the file ensures that both ends
>> of the propagate have the right type so it works.  However, if we relax the
>> restriction that both ends of the propagation have the same type, it ends
>> up causing us to bail early in cases we don't want.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> index 764741d..e8d092c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> @@ -307,7 +307,7 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>>      * instead. See also resolve_ud_negate() and comment in
>>      * fs_generator::generate_code.
>>      */
>> -   if (inst->src[arg].type == BRW_REGISTER_TYPE_UD &&
>> +   if (entry->src.type == BRW_REGISTER_TYPE_UD &&
>>         entry->src.negate)
>>        return false;
>>
>> --
>
> I think the claim is that we're mistakenly preventing something like
>
>    mov tmp:D, -src:D
>    cmp ..., tmp:UD, ...
>
> from being handled by copy propagation? If it were propagated, it
> would turn into
>
>    cmp ..., -src:UD, ...
>
> (By the way, an example like this in the commit message would make
> review a lot easier)
>
> Same page so far?
>
> As far as I can tell that transformation would be wrong. As I
> understand it, negating a UD value generates a value that potentially
> needs 33 bits to represent, and the comparison handles that temporary
> value properly, but if you negate and copy to a register (like the mov
> is doing) you lose the top bit.

The problem here is that when a source modifier is applied to a UD
value, a 33-bit representation is internally used.  If you do the
following:

mov foo:UD 7U
mov bar:UD -foo:UD
mov out:F bar:UD

the out register will have the value (float)(unt32_t)-7 which is some
very large floating-point number.  However, if we allow
copy-propagation of the second mov, we get

mov foo:UD 7U
mov out:f -bar:UD

and, since the negation is computed in 33-bits, we get a value of
-7.0f which is clearly not the same.  This is also a problem if we do
the negate in the source of a CMP.

The oriiginal code checked the final mov in our sequence to see if its
source was type UD and the second mov looking for the negation.

I'll add something like that to the commit message.
--Jason