[Mesa-dev] [PATCH 5/9] intel/compiler: relax brw_eu_validate for byte raw movs

[Chema Casanova]

El 23/1/19 a las 7:26, Matt Turner escribió:
> On Sun, Jul 8, 2018 at 5:27 PM, Jose Maria Casanova Crespo
> <jmcasanova at igalia.com> wrote:
>> When the destination is a BYTE type allow raw movs
>> even if the stride is not exact multiple of destination
>> type and exec type, execution type is Word and its size is 2.
>>
>> This restriction was only allowing stride==2 destinations
>> for 8-bit types.
> 
> Super late review, obviously... it's been on my todo list but fp64 was
> taking all my time.
> 
> I can't figure this commit out. What I know:
> 
>  - byte destination
>  - raw mov (which means destination stride == 1)

I think that stride == 1 is a requirement for a raw mov based on how it
is written at the PRM KBL Vol2A, "mov - Move" (Page 1081, PDF Page 1099).

"A mov with the same source and destination type, no source modifier,
and no saturation is a raw move."

But just after this text it is a reference for stride restriction  when
destination stride is 1 for byte types.

"A packed byte destination region (B or UB type with HorzStride == 1 and
ExecSize > 1) can only be written using raw move."

>  - execution type of a byte operation is "word"
> 
> The original code
> 
>>     if (exec_type_size > dst_type_size) {
>>        ERROR_IF(dst_stride * dst_type_size != exec_type_size,
>>                 "Destination stride must be equal to the ratio of the sizes of "
>>                 "the execution data type to the destination type");
>>     }
> 
> would not have worked for an instruction like
> 
>> mov(8)          g0<1>B          g0<8,8,1>B
> 
> But, that's okay because it didn't need to since the block right above
> it does this:
> 
>    if (dst_type_is_byte) {
>       if (is_packed(exec_size * dst_stride, exec_size, dst_stride)) {
>          if (!inst_is_raw_move(devinfo, inst)) {
>             ERROR("Only raw MOV supports a packed-byte destination");
>             return error_msg;
>          } else {
>             return (struct string){};
>          }
>       }
>    }
> 
> That is, if it's a raw move, return no-error.

Packed raw MOVs.

> It would be easier to understand what you were fixing if you had added
> a unit test to test_eu_validate.cpp, or (if my suspicions are correct)
> it would have proven to you that this patch wasn't correct.

Agree. And I should have also included in the commit log an example of
the kind of operations that we would like to allow.

> Was this just something that you noticed by inspection?

This issue was found because the validator was raising errors at the
shuffle/unshuffle operations when multiple 8-bit components were
prepared to be written as one 32-bit components for example for
store_ssbo So we had operations like these:

mov(8)         g9<4>B            g3<8,8,1>B    { align1 1H };
mov(8)         g9.1<4>B          g3.8<8,8,1>B    { align1 1H };
mov(8)         g9.2<4>B          g3.16<8,8,1>B    { align1 1H };
mov(8)         g9.3<4>B          g3.24<8,8,1>B    { align1 1H };

In theses case we have not packed raw movs, and the instructions worked
perfectly fine they were tested on the HW. But reading the PRM you would
have doubts if they were really allowed because as is stated
"Destination stride must be equal to the ratio of the sizes of the
execution data type to the destination type" and the execution size of 2
for bytes makes it more complex to understand.

I suppose that the think is that in this case the PRM seems to be
under-documented, and we can consider that this is implicitly allowed.
So we need to relax the validation.

>> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> 
> Sigh.