Re: [Mesa-dev] [PATCH v2 39/53] intel/compiler: add a helper to do conversions between integer and half-float
On Fri, 2019-01-04 at 02:02 -0800, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Wed, 2019-01-02 at 15:00 -0800, Francisco Jerez wrote:
> > > Iago Toral Quiroga writes:
> > >
> > > > There are hardware restrictions to consider that seem to affect
> > > > atom platforms
> > > > only.
> > >
> > > Same comment here as for PATCH 13 of this series. This and PATCH
> > > 40
> > > shouldn't be necessary anymore with [1] in place. Please drop
> > > them.
> >
> > Actually, I think your pass doesn't handle this case. I have just
> > tested this and I get various regressions for conversions between
> > integers (specifically integers lower than 32-bit, so I wonder if
> > this
> > restriction only affects these cases) and half-float.
> >
> > Here is an example of final IR generated with your pass and without
> > the
> > call to fixup_int_half_float_conversion from my series:
> >
> > mov(8) vgrf1+0.0:HF, vgrf0<2>:W
> >
> > Which should be:
> >
> > mov(8) vgrf1<2>:HF, vgrf0<2>:W
> >
> > It seems your pass doesn't act on this code since
> > INTEL_DEBUG=optimizer
> > doesn't show any trace of it.
> >
> > If this is not a bug in your pass and just that it doesn't handle
> > this
> > case, I am happy to add the support for it as part of my series if
> > that
> > makes sense to you, just let me know if that is the case.
> >
>
> It's not really a bug, you just need to add a case to
> has_dst_aligned_region_restriction() for it to return true for FP16
> instructions with this restriction, sorry I didn't point that out
> before.
Ok, I'll do that as part of my series then, thanks!
Iago
> > Iago
> >
> > > [1]
> > >
https://lists.freedesktop.org/archives/mesa-dev/2018-December/212775.html
> > >
> > > > ---
> > > > src/intel/compiler/brw_fs_nir.cpp | 32
> > > > +++
> > > > 1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > > > b/src/intel/compiler/brw_fs_nir.cpp
> > > > index 802f5cb0944..a9fd98bab68 100644
> > > > --- a/src/intel/compiler/brw_fs_nir.cpp
> > > > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > > > @@ -696,6 +696,38 @@ fixup_64bit_conversion(const fs_builder
> > > > &bld,
> > > > return false;
> > > > }
> > > >
> > > > +static bool
> > > > +fixup_int_half_float_conversion(const fs_builder &bld,
> > > > +fs_reg dst, fs_reg src,
> > > > +bool saturate,
> > > > +const struct gen_device_info
> > > > *devinfo)
> > > > +{
> > > > + /* CHV PRM, 3D Media GPGPU Engine, Register Region
> > > > Restrictions,
> > > > +* Special Restrictions:
> > > > +*
> > > > +*"Conversion between Integer and HF (Half Float) must
> > > > be
> > > > DWord
> > > > +* aligned and strided by a DWord on the destination."
> > > > +*
> > > > +* The same restriction is listed for other hardware
> > > > platforms,
> > > > however,
> > > > +* empirical testing suggests that only atom platforms are
> > > > affected.
> > > > +*/
> > > > + if (!devinfo->is_cherryview &&
> > > > !gen_device_info_is_9lp(devinfo))
> > > > + return false;
> > > > +
> > > > + if (!((dst.type == BRW_REGISTER_TYPE_HF &&
> > > > !brw_reg_type_is_floating_point(src.type)) ||
> > > > + (src.type == BRW_REGISTER_TYPE_HF &&
> > > > !brw_reg_type_is_floating_point(dst.type
> > > > + return false;
> > > > +
> > > > + fs_reg tmp =
> > > > horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F,
> > > > 1),
> > > > +dst.type),
> > > > + 2);
> > > > + bld.MOV(tmp, src);
> > > > + fs_inst *inst = bld.MOV(dst, tmp);
> > > > + inst->saturate = saturate;
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > void
> > > > fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr
> > > > *instr)
> > > > {
> > > > --
> > > > 2.17.1
> > > >
> > > > ___
> > > > mesa-dev mailing list
> > > > mesa-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 39/53] intel/compiler: add a helper to do conversions between integer and half-float
Iago Toral writes:
> On Wed, 2019-01-02 at 15:00 -0800, Francisco Jerez wrote:
>> Iago Toral Quiroga writes:
>>
>> > There are hardware restrictions to consider that seem to affect
>> > atom platforms
>> > only.
>>
>> Same comment here as for PATCH 13 of this series. This and PATCH 40
>> shouldn't be necessary anymore with [1] in place. Please drop them.
>
> Actually, I think your pass doesn't handle this case. I have just
> tested this and I get various regressions for conversions between
> integers (specifically integers lower than 32-bit, so I wonder if this
> restriction only affects these cases) and half-float.
>
> Here is an example of final IR generated with your pass and without the
> call to fixup_int_half_float_conversion from my series:
>
> mov(8) vgrf1+0.0:HF, vgrf0<2>:W
>
> Which should be:
>
> mov(8) vgrf1<2>:HF, vgrf0<2>:W
>
> It seems your pass doesn't act on this code since INTEL_DEBUG=optimizer
> doesn't show any trace of it.
>
> If this is not a bug in your pass and just that it doesn't handle this
> case, I am happy to add the support for it as part of my series if that
> makes sense to you, just let me know if that is the case.
>
It's not really a bug, you just need to add a case to
has_dst_aligned_region_restriction() for it to return true for FP16
instructions with this restriction, sorry I didn't point that out
before.
> Iago
>
>> [1]
>> https://lists.freedesktop.org/archives/mesa-dev/2018-December/212775.html
>>
>> > ---
>> > src/intel/compiler/brw_fs_nir.cpp | 32
>> > +++
>> > 1 file changed, 32 insertions(+)
>> >
>> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
>> > b/src/intel/compiler/brw_fs_nir.cpp
>> > index 802f5cb0944..a9fd98bab68 100644
>> > --- a/src/intel/compiler/brw_fs_nir.cpp
>> > +++ b/src/intel/compiler/brw_fs_nir.cpp
>> > @@ -696,6 +696,38 @@ fixup_64bit_conversion(const fs_builder &bld,
>> > return false;
>> > }
>> >
>> > +static bool
>> > +fixup_int_half_float_conversion(const fs_builder &bld,
>> > +fs_reg dst, fs_reg src,
>> > +bool saturate,
>> > +const struct gen_device_info
>> > *devinfo)
>> > +{
>> > + /* CHV PRM, 3D Media GPGPU Engine, Register Region
>> > Restrictions,
>> > +* Special Restrictions:
>> > +*
>> > +*"Conversion between Integer and HF (Half Float) must be
>> > DWord
>> > +* aligned and strided by a DWord on the destination."
>> > +*
>> > +* The same restriction is listed for other hardware platforms,
>> > however,
>> > +* empirical testing suggests that only atom platforms are
>> > affected.
>> > +*/
>> > + if (!devinfo->is_cherryview &&
>> > !gen_device_info_is_9lp(devinfo))
>> > + return false;
>> > +
>> > + if (!((dst.type == BRW_REGISTER_TYPE_HF &&
>> > !brw_reg_type_is_floating_point(src.type)) ||
>> > + (src.type == BRW_REGISTER_TYPE_HF &&
>> > !brw_reg_type_is_floating_point(dst.type
>> > + return false;
>> > +
>> > + fs_reg tmp = horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F,
>> > 1),
>> > +dst.type),
>> > + 2);
>> > + bld.MOV(tmp, src);
>> > + fs_inst *inst = bld.MOV(dst, tmp);
>> > + inst->saturate = saturate;
>> > +
>> > + return true;
>> > +}
>> > +
>> > void
>> > fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr
>> > *instr)
>> > {
>> > --
>> > 2.17.1
>> >
>> > ___
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 39/53] intel/compiler: add a helper to do conversions between integer and half-float
On Wed, 2019-01-02 at 15:00 -0800, Francisco Jerez wrote:
> Iago Toral Quiroga writes:
>
> > There are hardware restrictions to consider that seem to affect
> > atom platforms
> > only.
>
> Same comment here as for PATCH 13 of this series. This and PATCH 40
> shouldn't be necessary anymore with [1] in place. Please drop them.
Actually, I think your pass doesn't handle this case. I have just
tested this and I get various regressions for conversions between
integers (specifically integers lower than 32-bit, so I wonder if this
restriction only affects these cases) and half-float.
Here is an example of final IR generated with your pass and without the
call to fixup_int_half_float_conversion from my series:
mov(8) vgrf1+0.0:HF, vgrf0<2>:W
Which should be:
mov(8) vgrf1<2>:HF, vgrf0<2>:W
It seems your pass doesn't act on this code since INTEL_DEBUG=optimizer
doesn't show any trace of it.
If this is not a bug in your pass and just that it doesn't handle this
case, I am happy to add the support for it as part of my series if that
makes sense to you, just let me know if that is the case.
Iago
> [1]
> https://lists.freedesktop.org/archives/mesa-dev/2018-December/212775.html
>
> > ---
> > src/intel/compiler/brw_fs_nir.cpp | 32
> > +++
> > 1 file changed, 32 insertions(+)
> >
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > b/src/intel/compiler/brw_fs_nir.cpp
> > index 802f5cb0944..a9fd98bab68 100644
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > @@ -696,6 +696,38 @@ fixup_64bit_conversion(const fs_builder &bld,
> > return false;
> > }
> >
> > +static bool
> > +fixup_int_half_float_conversion(const fs_builder &bld,
> > +fs_reg dst, fs_reg src,
> > +bool saturate,
> > +const struct gen_device_info
> > *devinfo)
> > +{
> > + /* CHV PRM, 3D Media GPGPU Engine, Register Region
> > Restrictions,
> > +* Special Restrictions:
> > +*
> > +*"Conversion between Integer and HF (Half Float) must be
> > DWord
> > +* aligned and strided by a DWord on the destination."
> > +*
> > +* The same restriction is listed for other hardware platforms,
> > however,
> > +* empirical testing suggests that only atom platforms are
> > affected.
> > +*/
> > + if (!devinfo->is_cherryview &&
> > !gen_device_info_is_9lp(devinfo))
> > + return false;
> > +
> > + if (!((dst.type == BRW_REGISTER_TYPE_HF &&
> > !brw_reg_type_is_floating_point(src.type)) ||
> > + (src.type == BRW_REGISTER_TYPE_HF &&
> > !brw_reg_type_is_floating_point(dst.type
> > + return false;
> > +
> > + fs_reg tmp = horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F,
> > 1),
> > +dst.type),
> > + 2);
> > + bld.MOV(tmp, src);
> > + fs_inst *inst = bld.MOV(dst, tmp);
> > + inst->saturate = saturate;
> > +
> > + return true;
> > +}
> > +
> > void
> > fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr
> > *instr)
> > {
> > --
> > 2.17.1
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 39/53] intel/compiler: add a helper to do conversions between integer and half-float
Iago Toral Quiroga writes:
> There are hardware restrictions to consider that seem to affect atom platforms
> only.
Same comment here as for PATCH 13 of this series. This and PATCH 40
shouldn't be necessary anymore with [1] in place. Please drop them.
[1] https://lists.freedesktop.org/archives/mesa-dev/2018-December/212775.html
> ---
> src/intel/compiler/brw_fs_nir.cpp | 32 +++
> 1 file changed, 32 insertions(+)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 802f5cb0944..a9fd98bab68 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -696,6 +696,38 @@ fixup_64bit_conversion(const fs_builder &bld,
> return false;
> }
>
> +static bool
> +fixup_int_half_float_conversion(const fs_builder &bld,
> +fs_reg dst, fs_reg src,
> +bool saturate,
> +const struct gen_device_info *devinfo)
> +{
> + /* CHV PRM, 3D Media GPGPU Engine, Register Region Restrictions,
> +* Special Restrictions:
> +*
> +*"Conversion between Integer and HF (Half Float) must be DWord
> +* aligned and strided by a DWord on the destination."
> +*
> +* The same restriction is listed for other hardware platforms, however,
> +* empirical testing suggests that only atom platforms are affected.
> +*/
> + if (!devinfo->is_cherryview && !gen_device_info_is_9lp(devinfo))
> + return false;
> +
> + if (!((dst.type == BRW_REGISTER_TYPE_HF &&
> !brw_reg_type_is_floating_point(src.type)) ||
> + (src.type == BRW_REGISTER_TYPE_HF &&
> !brw_reg_type_is_floating_point(dst.type
> + return false;
> +
> + fs_reg tmp = horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F, 1),
> +dst.type),
> + 2);
> + bld.MOV(tmp, src);
> + fs_inst *inst = bld.MOV(dst, tmp);
> + inst->saturate = saturate;
> +
> + return true;
> +}
> +
> void
> fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
> {
> --
> 2.17.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 39/53] intel/compiler: add a helper to do conversions between integer and half-float
On Wed, Dec 19, 2018 at 12:51:07PM +0100, Iago Toral Quiroga wrote:
> There are hardware restrictions to consider that seem to affect atom platforms
> only.
> ---
> src/intel/compiler/brw_fs_nir.cpp | 32 +++
> 1 file changed, 32 insertions(+)
Reviewed-by: Topi Pohjolainen
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 802f5cb0944..a9fd98bab68 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -696,6 +696,38 @@ fixup_64bit_conversion(const fs_builder &bld,
> return false;
> }
>
> +static bool
> +fixup_int_half_float_conversion(const fs_builder &bld,
> +fs_reg dst, fs_reg src,
> +bool saturate,
> +const struct gen_device_info *devinfo)
> +{
> + /* CHV PRM, 3D Media GPGPU Engine, Register Region Restrictions,
> +* Special Restrictions:
> +*
> +*"Conversion between Integer and HF (Half Float) must be DWord
> +* aligned and strided by a DWord on the destination."
> +*
> +* The same restriction is listed for other hardware platforms, however,
> +* empirical testing suggests that only atom platforms are affected.
> +*/
> + if (!devinfo->is_cherryview && !gen_device_info_is_9lp(devinfo))
> + return false;
> +
> + if (!((dst.type == BRW_REGISTER_TYPE_HF &&
> !brw_reg_type_is_floating_point(src.type)) ||
> + (src.type == BRW_REGISTER_TYPE_HF &&
> !brw_reg_type_is_floating_point(dst.type
> + return false;
> +
> + fs_reg tmp = horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F, 1),
> +dst.type),
> + 2);
> + bld.MOV(tmp, src);
> + fs_inst *inst = bld.MOV(dst, tmp);
> + inst->saturate = saturate;
> +
> + return true;
> +}
> +
> void
> fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
> {
> --
> 2.17.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 39/53] intel/compiler: add a helper to do conversions between integer and half-float
There are hardware restrictions to consider that seem to affect atom platforms
only.
---
src/intel/compiler/brw_fs_nir.cpp | 32 +++
1 file changed, 32 insertions(+)
diff --git a/src/intel/compiler/brw_fs_nir.cpp
b/src/intel/compiler/brw_fs_nir.cpp
index 802f5cb0944..a9fd98bab68 100644
--- a/src/intel/compiler/brw_fs_nir.cpp
+++ b/src/intel/compiler/brw_fs_nir.cpp
@@ -696,6 +696,38 @@ fixup_64bit_conversion(const fs_builder &bld,
return false;
}
+static bool
+fixup_int_half_float_conversion(const fs_builder &bld,
+fs_reg dst, fs_reg src,
+bool saturate,
+const struct gen_device_info *devinfo)
+{
+ /* CHV PRM, 3D Media GPGPU Engine, Register Region Restrictions,
+* Special Restrictions:
+*
+*"Conversion between Integer and HF (Half Float) must be DWord
+* aligned and strided by a DWord on the destination."
+*
+* The same restriction is listed for other hardware platforms, however,
+* empirical testing suggests that only atom platforms are affected.
+*/
+ if (!devinfo->is_cherryview && !gen_device_info_is_9lp(devinfo))
+ return false;
+
+ if (!((dst.type == BRW_REGISTER_TYPE_HF &&
!brw_reg_type_is_floating_point(src.type)) ||
+ (src.type == BRW_REGISTER_TYPE_HF &&
!brw_reg_type_is_floating_point(dst.type
+ return false;
+
+ fs_reg tmp = horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F, 1),
+dst.type),
+ 2);
+ bld.MOV(tmp, src);
+ fs_inst *inst = bld.MOV(dst, tmp);
+ inst->saturate = saturate;
+
+ return true;
+}
+
void
fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
{
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
