[v3,28/42] intel/compiler: handle 64-bit float to 8-bit integer conversions

Submitted by Iago Toral Quiroga on Jan. 15, 2019, 1:54 p.m.

Details

Message ID 20190115135414.2313-29-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 5 4 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Jan. 15, 2019, 1:54 p.m.
These are not directly supported in hardware and brw_nir_lower_conversions
should have taken care of that before we get here. Also, while we are
at it, make sure 64-bit integer to 8-bit are also properly split by
the same lowering pass, since they have the same hardware restrictions.
---
 src/intel/compiler/brw_fs_nir.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp
index cf546b8ff09..e454578d99b 100644
--- a/src/intel/compiler/brw_fs_nir.cpp
+++ b/src/intel/compiler/brw_fs_nir.cpp
@@ -786,6 +786,10 @@  fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
    case nir_op_f2f16:
    case nir_op_i2f16:
    case nir_op_u2f16:
+   case nir_op_i2i8:
+   case nir_op_u2u8:
+   case nir_op_f2i8:
+   case nir_op_f2u8:
       assert(type_sz(op[0].type) < 8); /* brw_nir_lower_conversions */
       inst = bld.MOV(result, op[0]);
       inst->saturate = instr->dest.saturate;
@@ -824,8 +828,6 @@  fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
    case nir_op_u2u32:
    case nir_op_i2i16:
    case nir_op_u2u16:
-   case nir_op_i2i8:
-   case nir_op_u2u8:
       inst = bld.MOV(result, op[0]);
       inst->saturate = instr->dest.saturate;
       break;

Comments

This patch doesn't really do what the commit message says.  What it really
does is implement float -> 8-bit converions for *any* size float.

On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@igalia.com>
wrote:

> These are not directly supported in hardware and brw_nir_lower_conversions
> should have taken care of that before we get here. Also, while we are
> at it, make sure 64-bit integer to 8-bit are also properly split by
> the same lowering pass, since they have the same hardware restrictions.
>

Now that we have a lowering pass, having separate cases just so one of them
can assert seems silly.  If anything, we should just do

if (result.type == BRW_REGISTER_TYPE_B ||
    result.type == BRW_REGISTER_TYPE_UB ||
    result.type == BRW_REGISTER_TYPE_HF)
   assert(type_sz(op[0].type) < 8) /* brw_nir_lower_conversions */

and have it all in one big case.  The only special case we need is for
booleans where we need to negate them and fall through.

--Jason


> ---
>  src/intel/compiler/brw_fs_nir.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index cf546b8ff09..e454578d99b 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -786,6 +786,10 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
>     case nir_op_f2f16:
>     case nir_op_i2f16:
>     case nir_op_u2f16:
> +   case nir_op_i2i8:
> +   case nir_op_u2u8:
> +   case nir_op_f2i8:
> +   case nir_op_f2u8:
>        assert(type_sz(op[0].type) < 8); /* brw_nir_lower_conversions */
>        inst = bld.MOV(result, op[0]);
>        inst->saturate = instr->dest.saturate;
> @@ -824,8 +828,6 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
>     case nir_op_u2u32:
>     case nir_op_i2i16:
>     case nir_op_u2u16:
> -   case nir_op_i2i8:
> -   case nir_op_u2u8:
>        inst = bld.MOV(result, op[0]);
>        inst->saturate = instr->dest.saturate;
>        break;
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Thu, 2019-01-17 at 17:12 -0600, Jason Ekstrand wrote:
> This patch doesn't really do what the commit message says.  What it
> really does is implement float -> 8-bit converions for *any* size
> float.
> 
> On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@igalia.com
> > wrote:
> > These are not directly supported in hardware and
> > brw_nir_lower_conversions
> > 
> > should have taken care of that before we get here. Also, while we
> > are
> > 
> > at it, make sure 64-bit integer to 8-bit are also properly split by
> > 
> > the same lowering pass, since they have the same hardware
> > restrictions.
> 
> Now that we have a lowering pass, having separate cases just so one
> of them can assert seems silly.  If anything, we should just do
> 
> if (result.type == BRW_REGISTER_TYPE_B ||
>     result.type == BRW_REGISTER_TYPE_UB ||
>     result.type == BRW_REGISTER_TYPE_HF)
>    assert(type_sz(op[0].type) < 8) /* brw_nir_lower_conversions */
> 
> and have it all in one big case.  The only special case we need is
> for booleans where we need to negate them and fall through.

There are more cases, since the inverse conversions of these are not
supported either. I guess I'll just add this as well:
if (op[0].type == BRW_REGISTER_TYPE_B ||    op[0].type ==
BRW_REGISTER_TYPE_UB ||    op[0].type ==
BRW_REGISTER_TYPE_HF)   assert(type_sz(result.type) < 8); /*
brw_nir_lower_conversions */
> --Jason
>  
> > ---
> > 
> >  src/intel/compiler/brw_fs_nir.cpp | 6 ++++--
> > 
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > b/src/intel/compiler/brw_fs_nir.cpp
> > 
> > index cf546b8ff09..e454578d99b 100644
> > 
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > 
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > 
> > @@ -786,6 +786,10 @@ fs_visitor::nir_emit_alu(const fs_builder
> > &bld, nir_alu_instr *instr)
> > 
> >     case nir_op_f2f16:
> > 
> >     case nir_op_i2f16:
> > 
> >     case nir_op_u2f16:
> > 
> > +   case nir_op_i2i8:
> > 
> > +   case nir_op_u2u8:
> > 
> > +   case nir_op_f2i8:
> > 
> > +   case nir_op_f2u8:
> > 
> >        assert(type_sz(op[0].type) < 8); /*
> > brw_nir_lower_conversions */
> > 
> >        inst = bld.MOV(result, op[0]);
> > 
> >        inst->saturate = instr->dest.saturate;
> > 
> > @@ -824,8 +828,6 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> > nir_alu_instr *instr)
> > 
> >     case nir_op_u2u32:
> > 
> >     case nir_op_i2i16:
> > 
> >     case nir_op_u2u16:
> > 
> > -   case nir_op_i2i8:
> > 
> > -   case nir_op_u2u8:
> > 
> >        inst = bld.MOV(result, op[0]);
> > 
> >        inst->saturate = instr->dest.saturate;
> > 
> >        break;
> >
On Fri, 2019-01-18 at 12:13 +0100, Iago Toral wrote:
> On Thu, 2019-01-17 at 17:12 -0600, Jason Ekstrand wrote:
> > This patch doesn't really do what the commit message says.  What it
> > really does is implement float -> 8-bit converions for *any* size
> > float.
> > 
> > On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <
> > itoral@igalia.com> wrote:
> > > These are not directly supported in hardware and
> > > brw_nir_lower_conversions
> > > 
> > > should have taken care of that before we get here. Also, while we
> > > are
> > > 
> > > at it, make sure 64-bit integer to 8-bit are also properly split
> > > by
> > > 
> > > the same lowering pass, since they have the same hardware
> > > restrictions.
> > 
> > Now that we have a lowering pass, having separate cases just so one
> > of them can assert seems silly.  If anything, we should just do
> > 
> > if (result.type == BRW_REGISTER_TYPE_B ||
> >     result.type == BRW_REGISTER_TYPE_UB ||
> >     result.type == BRW_REGISTER_TYPE_HF)
> >    assert(type_sz(op[0].type) < 8) /* brw_nir_lower_conversions */
> > 
> > and have it all in one big case.  The only special case we need is
> > for booleans where we need to negate them and fall through.
> 
> There are more cases, since the inverse conversions of these are not
> supported either. I guess I'll just add this as well:
> 
> if (op[0].type == BRW_REGISTER_TYPE_B ||
>     op[0].type == BRW_REGISTER_TYPE_UB ||
>     op[0].type == BRW_REGISTER_TYPE_HF)
>    assert(type_sz(result.type) < 8); /* brw_nir_lower_conversions */

Oh, and there is also the rounding opcodes for f16 destinations (plus
the big comment about brw_F32TO16 that comes with their fallthrough)...
I think we might want to keep the three f2f16* opcodes as a separate
block and do as you suggest for the remaining ones if that is okay.
> > --Jason
> >  
> > > ---
> > > 
> > >  src/intel/compiler/brw_fs_nir.cpp | 6 ++++--
> > > 
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > 
> > > 
> > > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > > b/src/intel/compiler/brw_fs_nir.cpp
> > > 
> > > index cf546b8ff09..e454578d99b 100644
> > > 
> > > --- a/src/intel/compiler/brw_fs_nir.cpp
> > > 
> > > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > > 
> > > @@ -786,6 +786,10 @@ fs_visitor::nir_emit_alu(const fs_builder
> > > &bld, nir_alu_instr *instr)
> > > 
> > >     case nir_op_f2f16:
> > > 
> > >     case nir_op_i2f16:
> > > 
> > >     case nir_op_u2f16:
> > > 
> > > +   case nir_op_i2i8:
> > > 
> > > +   case nir_op_u2u8:
> > > 
> > > +   case nir_op_f2i8:
> > > 
> > > +   case nir_op_f2u8:
> > > 
> > >        assert(type_sz(op[0].type) < 8); /*
> > > brw_nir_lower_conversions */
> > > 
> > >        inst = bld.MOV(result, op[0]);
> > > 
> > >        inst->saturate = instr->dest.saturate;
> > > 
> > > @@ -824,8 +828,6 @@ fs_visitor::nir_emit_alu(const fs_builder
> > > &bld, nir_alu_instr *instr)
> > > 
> > >     case nir_op_u2u32:
> > > 
> > >     case nir_op_i2i16:
> > > 
> > >     case nir_op_u2u16:
> > > 
> > > -   case nir_op_i2i8:
> > > 
> > > -   case nir_op_u2u8:
> > > 
> > >        inst = bld.MOV(result, op[0]);
> > > 
> > >        inst->saturate = instr->dest.saturate;
> > > 
> > >        break;
> > >
On January 18, 2019 06:23:35 Iago Toral <itoral@igalia.com> wrote:
> On Fri, 2019-01-18 at 12:13 +0100, Iago Toral wrote:
>> On Thu, 2019-01-17 at 17:12 -0600, Jason Ekstrand wrote:
>>> This patch doesn't really do what the commit message says.  What it really 
>>> does is implement float -> 8-bit converions for *any* size float.
>>>
>>> On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@igalia.com> wrote:
>>>> These are not directly supported in hardware and brw_nir_lower_conversions
>>>> should have taken care of that before we get here. Also, while we are
>>>> at it, make sure 64-bit integer to 8-bit are also properly split by
>>>> the same lowering pass, since they have the same hardware restrictions.
>>>
>>> Now that we have a lowering pass, having separate cases just so one of them 
>>> can assert seems silly.  If anything, we should just do
>>>
>>> if (result.type == BRW_REGISTER_TYPE_B ||
>>>    result.type == BRW_REGISTER_TYPE_UB ||
>>>    result.type == BRW_REGISTER_TYPE_HF)
>>>   assert(type_sz(op[0].type) < 8) /* brw_nir_lower_conversions */
>>>
>>> and have it all in one big case.  The only special case we need is for 
>>> booleans where we need to negate them and fall through.
>>
>> There are more cases, since the inverse conversions of these are not 
>> supported either. I guess I'll just add this as well:
>>
>> if (op[0].type == BRW_REGISTER_TYPE_B ||
>> op[0].type == BRW_REGISTER_TYPE_UB ||
>> op[0].type == BRW_REGISTER_TYPE_HF)
>> assert(type_sz(result.type) < 8); /* brw_nir_lower_conversions */
>
> Oh, and there is also the rounding opcodes for f16 destinations (plus the 
> big comment about brw_F32TO16 that comes with their fallthrough)... I think 
> we might want to keep the three f2f16* opcodes as a separate block and do 
> as you suggest for the remaining ones if that is okay.

That's fine as they actually do do something different.

-Jason

>
>>> --Jason
>>>
>>>> ---
>>>> src/intel/compiler/brw_fs_nir.cpp | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/intel/compiler/brw_fs_nir.cpp 
>>>> b/src/intel/compiler/brw_fs_nir.cpp
>>>> index cf546b8ff09..e454578d99b 100644
>>>> --- a/src/intel/compiler/brw_fs_nir.cpp
>>>> +++ b/src/intel/compiler/brw_fs_nir.cpp
>>>> @@ -786,6 +786,10 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, 
>>>> nir_alu_instr *instr)
>>>>    case nir_op_f2f16:
>>>>    case nir_op_i2f16:
>>>>    case nir_op_u2f16:
>>>> +   case nir_op_i2i8:
>>>> +   case nir_op_u2u8:
>>>> +   case nir_op_f2i8:
>>>> +   case nir_op_f2u8:
>>>>       assert(type_sz(op[0].type) < 8); /* brw_nir_lower_conversions */
>>>>       inst = bld.MOV(result, op[0]);
>>>>       inst->saturate = instr->dest.saturate;
>>>> @@ -824,8 +828,6 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, 
>>>> nir_alu_instr *instr)
>>>>    case nir_op_u2u32:
>>>>    case nir_op_i2i16:
>>>>    case nir_op_u2u16:
>>>> -   case nir_op_i2i8:
>>>> -   case nir_op_u2u8:
>>>>       inst = bld.MOV(result, op[0]);
>>>>       inst->saturate = instr->dest.saturate;
>>>>       break;