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

[Jason Ekstrand]

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>

Thanks for fixing that.  I suppose we can probably revert my "fix" with the 
strides then...

--Jason