[Mesa-dev] [PATCH] i965/fs: Fix extract_i8/u8 to a 64-bit destination

[Matt Turner]

On Mon, Nov 13, 2017 at 11:20 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On November 13, 2017 22:54:02 Matt Turner <mattst88 at gmail.com> wrote:
>
>> On Mon, Nov 13, 2017 at 10:06 PM, Jason Ekstrand <jason at jlekstrand.net>
>> wrote:
>>>
>>> On Mon, Nov 13, 2017 at 3:23 PM, Matt Turner <mattst88 at gmail.com> wrote:
>>>>
>>>>
>>>> The MOV instruction can extract bytes to words/double words, and
>>>> words/double words to quadwords, but not byte to quadwords.
>>>>
>>>> For unsigned byte to quadword, we can read them as words and AND off the
>>>> high byte and extract to quadword in one instruction. For signed bytes,
>>>> we need to first sign extend to word and the sign extend that word to a
>>>> quadword.
>>>>
>>>> Fixes the following test on CHV, BXT, and GLK:
>>>>    KHR-GL46.shader_ballot_tests.ShaderBallotBitmasks
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103628
>>>> ---
>>>>  src/intel/compiler/brw_fs_nir.cpp | 20 ++++++++++++++++++--
>>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/intel/compiler/brw_fs_nir.cpp
>>>> b/src/intel/compiler/brw_fs_nir.cpp
>>>> index 38d0d357e8..e266837ece 100644
>>>> --- a/src/intel/compiler/brw_fs_nir.cpp
>>>> +++ b/src/intel/compiler/brw_fs_nir.cpp
>>>> @@ -1395,10 +1395,26 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
>>>> nir_alu_instr *instr)
>>>>
>>>>     case nir_op_extract_u8:
>>>>     case nir_op_extract_i8: {
>>>> -      const brw_reg_type type = brw_int_type(1, instr->op ==
>>>> nir_op_extract_i8);
>>>>        nir_const_value *byte =
>>>> nir_src_as_const_value(instr->src[1].src);
>>>>        assert(byte != NULL);
>>>> -      bld.MOV(result, subscript(op[0], type, byte->u32[0]));
>>>> +
>>>> +      /* MOV does not support a 64-bit destination and a byte source */
>>>> +      if (nir_dest_bit_size(instr->dest.dest) == 64) {
>>>
>>>
>>>
>>> Do we also want to predicate this on "devinfo->is_cherrytrail ||
>>> gen_device_info_is_gen9lp(devinfo)" as well as bit size?  Big-core can
>>> handle it just fine.
>>
>>
>> Actually, the documentation says it isn't allowed:
>>
>> BDW+
>> There is no direct conversion from B/UB to Q/UQ or Q/UQ to B/UB. Use
>> two instructions and a word or DWord intermediate integer type.
>
>
> Interesting.  Mind throwing that in the comment above?  With that,
>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

Definitely. Thanks. I actually didn't realize there was such a
statement in the documentation until I double checked last night.
Previously I was just looking at all of the allowed src/dest type
combinations.