Re: [Mesa-dev] [PATCH v2 13/53] intel/compiler: add a helper to handle conversions to 64-bit in atom
Thanks Curro, I'll check it out.
Iago
On Wed, 2019-01-02 at 15:00 -0800, Francisco Jerez wrote:
> This patch is redundant with the regioning lowering pass I sent a few
> days ago [1]. The problem with this approach is that on the one hand
> it's easy for the back-end compiler to cause code which was legalized
> at
> NIR translation time to become illegal again accidentally, on the
> other
> hand there's the maintainability issue of having workarounds for the
> exact same restriction scattered all over the place.
>
> Please drop it, there shouldn't be any need to do manual workarounds
> at
> NIR translation time for the CHV/BXT regioning restrictions to be
> honored anymore.
>
> [1]
> https://lists.freedesktop.org/archives/mesa-dev/2018-December/212775.html
>
> Iago Toral Quiroga writes:
>
> > ---
> > src/intel/compiler/brw_fs_nir.cpp | 55 ++-
> >
> > 1 file changed, 33 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > b/src/intel/compiler/brw_fs_nir.cpp
> > index 92ec85a27cc..15715651aa6 100644
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > @@ -664,6 +664,38 @@ brw_rnd_mode_from_nir_op (const nir_op op) {
> > }
> > }
> >
> > +static bool
> > +fixup_64bit_conversion(const fs_builder &bld,
> > + fs_reg dst, fs_reg src,
> > + bool saturate,
> > + const struct gen_device_info *devinfo)
> > +{
> > + /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region
> > Restrictions:
> > +*
> > +*"When source or destination is 64b (...), regioning in
> > Align1
> > +* must follow these rules:
> > +*
> > +* 1. Source and destination horizontal stride must be
> > aligned to
> > +*the same qword.
> > +* (...)"
> > +*
> > +* This means that conversions from bit-sizes smaller than 64-
> > bit to
> > +* 64-bit need to have the source data elements aligned to 64-
> > bit.
> > +* This restriction does not apply to BDW and later.
> > +*/
> > + if (type_sz(dst.type) == 8 && type_sz(src.type) < 8 &&
> > + (devinfo->is_cherryview ||
> > gen_device_info_is_9lp(devinfo))) {
> > + fs_reg tmp = bld.vgrf(dst.type, 1);
> > + tmp = subscript(tmp, src.type, 0);
> > + bld.MOV(tmp, src);
> > + fs_inst *inst = bld.MOV(dst, tmp);
> > + inst->saturate = saturate;
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > void
> > fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr
> > *instr)
> > {
> > @@ -805,29 +837,8 @@ fs_visitor::nir_emit_alu(const fs_builder
> > &bld, nir_alu_instr *instr)
> > case nir_op_i2i64:
> > case nir_op_u2f64:
> > case nir_op_u2u64:
> > - /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region
> > Restrictions:
> > - *
> > - *"When source or destination is 64b (...), regioning in
> > Align1
> > - * must follow these rules:
> > - *
> > - * 1. Source and destination horizontal stride must be
> > aligned to
> > - *the same qword.
> > - * (...)"
> > - *
> > - * This means that conversions from bit-sizes smaller than
> > 64-bit to
> > - * 64-bit need to have the source data elements aligned to
> > 64-bit.
> > - * This restriction does not apply to BDW and later.
> > - */
> > - if (nir_dest_bit_size(instr->dest.dest) == 64 &&
> > - nir_src_bit_size(instr->src[0].src) < 64 &&
> > - (devinfo->is_cherryview ||
> > gen_device_info_is_9lp(devinfo))) {
> > - fs_reg tmp = bld.vgrf(result.type, 1);
> > - tmp = subscript(tmp, op[0].type, 0);
> > - inst = bld.MOV(tmp, op[0]);
> > - inst = bld.MOV(result, tmp);
> > - inst->saturate = instr->dest.saturate;
> > + if (fixup_64bit_conversion(bld, result, op[0], instr-
> > >dest.saturate, devinfo))
> > break;
> > - }
> >/* fallthrough */
> > case nir_op_f2f32:
> > case nir_op_f2i32:
> > --
> > 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 13/53] intel/compiler: add a helper to handle conversions to 64-bit in atom
This patch is redundant with the regioning lowering pass I sent a few
days ago [1]. The problem with this approach is that on the one hand
it's easy for the back-end compiler to cause code which was legalized at
NIR translation time to become illegal again accidentally, on the other
hand there's the maintainability issue of having workarounds for the
exact same restriction scattered all over the place.
Please drop it, there shouldn't be any need to do manual workarounds at
NIR translation time for the CHV/BXT regioning restrictions to be
honored anymore.
[1] https://lists.freedesktop.org/archives/mesa-dev/2018-December/212775.html
Iago Toral Quiroga writes:
> ---
> src/intel/compiler/brw_fs_nir.cpp | 55 ++-
> 1 file changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 92ec85a27cc..15715651aa6 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -664,6 +664,38 @@ brw_rnd_mode_from_nir_op (const nir_op op) {
> }
> }
>
> +static bool
> +fixup_64bit_conversion(const fs_builder &bld,
> + fs_reg dst, fs_reg src,
> + bool saturate,
> + const struct gen_device_info *devinfo)
> +{
> + /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region Restrictions:
> +*
> +*"When source or destination is 64b (...), regioning in Align1
> +* must follow these rules:
> +*
> +* 1. Source and destination horizontal stride must be aligned to
> +*the same qword.
> +* (...)"
> +*
> +* This means that conversions from bit-sizes smaller than 64-bit to
> +* 64-bit need to have the source data elements aligned to 64-bit.
> +* This restriction does not apply to BDW and later.
> +*/
> + if (type_sz(dst.type) == 8 && type_sz(src.type) < 8 &&
> + (devinfo->is_cherryview || gen_device_info_is_9lp(devinfo))) {
> + fs_reg tmp = bld.vgrf(dst.type, 1);
> + tmp = subscript(tmp, src.type, 0);
> + bld.MOV(tmp, src);
> + fs_inst *inst = bld.MOV(dst, tmp);
> + inst->saturate = saturate;
> + return true;
> + }
> +
> + return false;
> +}
> +
> void
> fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
> {
> @@ -805,29 +837,8 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
> case nir_op_i2i64:
> case nir_op_u2f64:
> case nir_op_u2u64:
> - /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region Restrictions:
> - *
> - *"When source or destination is 64b (...), regioning in Align1
> - * must follow these rules:
> - *
> - * 1. Source and destination horizontal stride must be aligned to
> - *the same qword.
> - * (...)"
> - *
> - * This means that conversions from bit-sizes smaller than 64-bit to
> - * 64-bit need to have the source data elements aligned to 64-bit.
> - * This restriction does not apply to BDW and later.
> - */
> - if (nir_dest_bit_size(instr->dest.dest) == 64 &&
> - nir_src_bit_size(instr->src[0].src) < 64 &&
> - (devinfo->is_cherryview || gen_device_info_is_9lp(devinfo))) {
> - fs_reg tmp = bld.vgrf(result.type, 1);
> - tmp = subscript(tmp, op[0].type, 0);
> - inst = bld.MOV(tmp, op[0]);
> - inst = bld.MOV(result, tmp);
> - inst->saturate = instr->dest.saturate;
> + if (fixup_64bit_conversion(bld, result, op[0], instr->dest.saturate,
> devinfo))
> break;
> - }
>/* fallthrough */
> case nir_op_f2f32:
> case nir_op_f2i32:
> --
> 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 13/53] intel/compiler: add a helper to handle conversions to 64-bit in atom
On Wed, Dec 19, 2018 at 12:50:41PM +0100, Iago Toral Quiroga wrote:
> ---
> src/intel/compiler/brw_fs_nir.cpp | 55 ++-
> 1 file changed, 33 insertions(+), 22 deletions(-)
Reviewed-by: Topi Pohjolainen
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 92ec85a27cc..15715651aa6 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -664,6 +664,38 @@ brw_rnd_mode_from_nir_op (const nir_op op) {
> }
> }
>
> +static bool
> +fixup_64bit_conversion(const fs_builder &bld,
> + fs_reg dst, fs_reg src,
> + bool saturate,
> + const struct gen_device_info *devinfo)
> +{
> + /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region Restrictions:
> +*
> +*"When source or destination is 64b (...), regioning in Align1
> +* must follow these rules:
> +*
> +* 1. Source and destination horizontal stride must be aligned to
> +*the same qword.
> +* (...)"
> +*
> +* This means that conversions from bit-sizes smaller than 64-bit to
> +* 64-bit need to have the source data elements aligned to 64-bit.
> +* This restriction does not apply to BDW and later.
> +*/
> + if (type_sz(dst.type) == 8 && type_sz(src.type) < 8 &&
> + (devinfo->is_cherryview || gen_device_info_is_9lp(devinfo))) {
> + fs_reg tmp = bld.vgrf(dst.type, 1);
> + tmp = subscript(tmp, src.type, 0);
> + bld.MOV(tmp, src);
> + fs_inst *inst = bld.MOV(dst, tmp);
> + inst->saturate = saturate;
> + return true;
> + }
> +
> + return false;
> +}
> +
> void
> fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
> {
> @@ -805,29 +837,8 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
> case nir_op_i2i64:
> case nir_op_u2f64:
> case nir_op_u2u64:
> - /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region Restrictions:
> - *
> - *"When source or destination is 64b (...), regioning in Align1
> - * must follow these rules:
> - *
> - * 1. Source and destination horizontal stride must be aligned to
> - *the same qword.
> - * (...)"
> - *
> - * This means that conversions from bit-sizes smaller than 64-bit to
> - * 64-bit need to have the source data elements aligned to 64-bit.
> - * This restriction does not apply to BDW and later.
> - */
> - if (nir_dest_bit_size(instr->dest.dest) == 64 &&
> - nir_src_bit_size(instr->src[0].src) < 64 &&
> - (devinfo->is_cherryview || gen_device_info_is_9lp(devinfo))) {
> - fs_reg tmp = bld.vgrf(result.type, 1);
> - tmp = subscript(tmp, op[0].type, 0);
> - inst = bld.MOV(tmp, op[0]);
> - inst = bld.MOV(result, tmp);
> - inst->saturate = instr->dest.saturate;
> + if (fixup_64bit_conversion(bld, result, op[0], instr->dest.saturate,
> devinfo))
> break;
> - }
>/* fallthrough */
> case nir_op_f2f32:
> case nir_op_f2i32:
> --
> 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 13/53] intel/compiler: add a helper to handle conversions to 64-bit in atom
---
src/intel/compiler/brw_fs_nir.cpp | 55 ++-
1 file changed, 33 insertions(+), 22 deletions(-)
diff --git a/src/intel/compiler/brw_fs_nir.cpp
b/src/intel/compiler/brw_fs_nir.cpp
index 92ec85a27cc..15715651aa6 100644
--- a/src/intel/compiler/brw_fs_nir.cpp
+++ b/src/intel/compiler/brw_fs_nir.cpp
@@ -664,6 +664,38 @@ brw_rnd_mode_from_nir_op (const nir_op op) {
}
}
+static bool
+fixup_64bit_conversion(const fs_builder &bld,
+ fs_reg dst, fs_reg src,
+ bool saturate,
+ const struct gen_device_info *devinfo)
+{
+ /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region Restrictions:
+*
+*"When source or destination is 64b (...), regioning in Align1
+* must follow these rules:
+*
+* 1. Source and destination horizontal stride must be aligned to
+*the same qword.
+* (...)"
+*
+* This means that conversions from bit-sizes smaller than 64-bit to
+* 64-bit need to have the source data elements aligned to 64-bit.
+* This restriction does not apply to BDW and later.
+*/
+ if (type_sz(dst.type) == 8 && type_sz(src.type) < 8 &&
+ (devinfo->is_cherryview || gen_device_info_is_9lp(devinfo))) {
+ fs_reg tmp = bld.vgrf(dst.type, 1);
+ tmp = subscript(tmp, src.type, 0);
+ bld.MOV(tmp, src);
+ fs_inst *inst = bld.MOV(dst, tmp);
+ inst->saturate = saturate;
+ return true;
+ }
+
+ return false;
+}
+
void
fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
{
@@ -805,29 +837,8 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
nir_alu_instr *instr)
case nir_op_i2i64:
case nir_op_u2f64:
case nir_op_u2u64:
- /* CHV PRM, vol07, 3D Media GPGPU Engine, Register Region Restrictions:
- *
- *"When source or destination is 64b (...), regioning in Align1
- * must follow these rules:
- *
- * 1. Source and destination horizontal stride must be aligned to
- *the same qword.
- * (...)"
- *
- * This means that conversions from bit-sizes smaller than 64-bit to
- * 64-bit need to have the source data elements aligned to 64-bit.
- * This restriction does not apply to BDW and later.
- */
- if (nir_dest_bit_size(instr->dest.dest) == 64 &&
- nir_src_bit_size(instr->src[0].src) < 64 &&
- (devinfo->is_cherryview || gen_device_info_is_9lp(devinfo))) {
- fs_reg tmp = bld.vgrf(result.type, 1);
- tmp = subscript(tmp, op[0].type, 0);
- inst = bld.MOV(tmp, op[0]);
- inst = bld.MOV(result, tmp);
- inst->saturate = instr->dest.saturate;
+ if (fixup_64bit_conversion(bld, result, op[0], instr->dest.saturate,
devinfo))
break;
- }
/* fallthrough */
case nir_op_f2f32:
case nir_op_f2i32:
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
