Re: [Mesa-dev] [PATCHv2 07/10] intel/fs: Introduce regioning lowering pass.
On Mon, 2019-01-07 at 12:02 -0800, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Sat, 2019-01-05 at 14:03 -0800, Francisco Jerez wrote:
> > > This legalization pass is meant to handle situations where the
> > > source
> > > or destination regioning controls of an instruction are
> > > unsupported
> > > by
> > > the hardware and need to be lowered away into separate
> > > instructions.
> > > This should be more reliable and future-proof than the current
> > > approach of handling CHV/BXT restrictions manually all over the
> > > visitor. The same mechanism is leveraged to lower unsupported
> > > type
> > > conversions easily, which obsoletes the lower_conversions pass.
> > >
> > > v2: Give conditional modifiers the same treatment as predicates
> > > for
> > > SEL instructions in lower_dst_modifiers() (Iago). Special-
> > > case a
> > > couple of other instructions with inconsistent conditional
> > > mod
> > > semantics in lower_dst_modifiers() (Curro).
> > > ---
> > > src/intel/Makefile.sources| 1 +
> > > src/intel/compiler/brw_fs.cpp | 5 +-
> > > src/intel/compiler/brw_fs.h | 21 +-
> > > src/intel/compiler/brw_fs_lower_regioning.cpp | 399
> > > ++
> > > src/intel/compiler/brw_ir_fs.h| 10 +
> > > src/intel/compiler/meson.build| 1 +
> > > 6 files changed, 418 insertions(+), 19 deletions(-)
> > > create mode 100644 src/intel/compiler/brw_fs_lower_regioning.cpp
> > >
> > > diff --git a/src/intel/Makefile.sources
> > > b/src/intel/Makefile.sources
> > > index 5e7d32293b7..6b9874d2b80 100644
> > > --- a/src/intel/Makefile.sources
> > > +++ b/src/intel/Makefile.sources
> > > @@ -64,6 +64,7 @@ COMPILER_FILES = \
> > > compiler/brw_fs_live_variables.h \
> > > compiler/brw_fs_lower_conversions.cpp \
> > > compiler/brw_fs_lower_pack.cpp \
> > > + compiler/brw_fs_lower_regioning.cpp \
> > > compiler/brw_fs_nir.cpp \
> > > compiler/brw_fs_reg_allocate.cpp \
> > > compiler/brw_fs_register_coalesce.cpp \
> > > diff --git a/src/intel/compiler/brw_fs.cpp
> > > b/src/intel/compiler/brw_fs.cpp
> > > index 889509badab..caa7a798332 100644
> > > --- a/src/intel/compiler/brw_fs.cpp
> > > +++ b/src/intel/compiler/brw_fs.cpp
> > > @@ -6471,7 +6471,10 @@ fs_visitor::optimize()
> > >OPT(dead_code_eliminate);
> > > }
> > >
> > > - if (OPT(lower_conversions)) {
> > > + progress = false;
> > > + OPT(lower_conversions);
> > > + OPT(lower_regioning);
> > > + if (progress) {
> >
> > This is a small nitpick but since this makes lower_conversions
> > redundant, maybe it makes more sense to just remove the call to it
> > here
> > already in this patch so you don't have to reset the progress
> > variable
> > and simply do:
> >
> > if (OPT(lower_regioning)) {
> >...
> > }
> >
>
> The main reason for this is that in the event of a regression this
> will
> allow identifying from the bisection result whether the reason for
> the
> failure is the lack of a condition in the lower_regioning pass which
> was
> previously handled by lower_conversions, or whether it's a bug in the
> lowering code of lower_regioning itself.
That's actually a good idea.
> > >OPT(opt_copy_propagation);
> > >OPT(dead_code_eliminate);
> > >OPT(lower_simd_width);
> > > diff --git a/src/intel/compiler/brw_fs.h
> > > b/src/intel/compiler/brw_fs.h
> > > index dc36ecc21ac..36825754931 100644
> > > --- a/src/intel/compiler/brw_fs.h
> > > +++ b/src/intel/compiler/brw_fs.h
> > > @@ -164,6 +164,7 @@ public:
> > > void lower_uniform_pull_constant_loads();
> > > bool lower_load_payload();
> > > bool lower_pack();
> > > + bool lower_regioning();
> > > bool lower_conversions();
> > > bool lower_logical_sends();
> > > bool lower_integer_multiplication();
> > > @@ -536,24 +537,8 @@ namespace brw {
> > >}
> > > }
> > >
> > > - /**
> > > -* Remove any modifiers from the \p i-th source region of the
> > > instruction,
> > > -* including negate, abs and any implicit type conversion to
> > > the
> > > execution
> > > -* type. Instead any source modifiers will be implemented as
> > > a
> > > separate
> > > -* MOV instruction prior to the original instruction.
> > > -*/
> > > - inline bool
> > > - lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst
> > > *inst, unsigned i)
> > > - {
> > > - assert(inst->components_read(i) == 1);
> > > - const fs_builder ibld(v, block, inst);
> > > - const fs_reg tmp = ibld.vgrf(get_exec_type(inst));
> > > -
> > > - ibld.MOV(tmp, inst->src[i]);
> > > - inst->src[i] = tmp;
> > > -
> > > - return true;
> > > - }
> > > + bool
> > > + lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst
> > > *inst, unsigned i);
> > > }
> > >
> > > void shuffle_from_32bit_read(const brw::fs_builder ,
> > > diff --git
Re: [Mesa-dev] [PATCHv2 07/10] intel/fs: Introduce regioning lowering pass.
Iago Toral writes:
> On Sat, 2019-01-05 at 14:03 -0800, Francisco Jerez wrote:
>> This legalization pass is meant to handle situations where the source
>> or destination regioning controls of an instruction are unsupported
>> by
>> the hardware and need to be lowered away into separate instructions.
>> This should be more reliable and future-proof than the current
>> approach of handling CHV/BXT restrictions manually all over the
>> visitor. The same mechanism is leveraged to lower unsupported type
>> conversions easily, which obsoletes the lower_conversions pass.
>>
>> v2: Give conditional modifiers the same treatment as predicates for
>> SEL instructions in lower_dst_modifiers() (Iago). Special-case a
>> couple of other instructions with inconsistent conditional mod
>> semantics in lower_dst_modifiers() (Curro).
>> ---
>> src/intel/Makefile.sources| 1 +
>> src/intel/compiler/brw_fs.cpp | 5 +-
>> src/intel/compiler/brw_fs.h | 21 +-
>> src/intel/compiler/brw_fs_lower_regioning.cpp | 399
>> ++
>> src/intel/compiler/brw_ir_fs.h| 10 +
>> src/intel/compiler/meson.build| 1 +
>> 6 files changed, 418 insertions(+), 19 deletions(-)
>> create mode 100644 src/intel/compiler/brw_fs_lower_regioning.cpp
>>
>> diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
>> index 5e7d32293b7..6b9874d2b80 100644
>> --- a/src/intel/Makefile.sources
>> +++ b/src/intel/Makefile.sources
>> @@ -64,6 +64,7 @@ COMPILER_FILES = \
>> compiler/brw_fs_live_variables.h \
>> compiler/brw_fs_lower_conversions.cpp \
>> compiler/brw_fs_lower_pack.cpp \
>> +compiler/brw_fs_lower_regioning.cpp \
>> compiler/brw_fs_nir.cpp \
>> compiler/brw_fs_reg_allocate.cpp \
>> compiler/brw_fs_register_coalesce.cpp \
>> diff --git a/src/intel/compiler/brw_fs.cpp
>> b/src/intel/compiler/brw_fs.cpp
>> index 889509badab..caa7a798332 100644
>> --- a/src/intel/compiler/brw_fs.cpp
>> +++ b/src/intel/compiler/brw_fs.cpp
>> @@ -6471,7 +6471,10 @@ fs_visitor::optimize()
>>OPT(dead_code_eliminate);
>> }
>>
>> - if (OPT(lower_conversions)) {
>> + progress = false;
>> + OPT(lower_conversions);
>> + OPT(lower_regioning);
>> + if (progress) {
>
> This is a small nitpick but since this makes lower_conversions
> redundant, maybe it makes more sense to just remove the call to it here
> already in this patch so you don't have to reset the progress variable
> and simply do:
>
> if (OPT(lower_regioning)) {
>...
> }
>
The main reason for this is that in the event of a regression this will
allow identifying from the bisection result whether the reason for the
failure is the lack of a condition in the lower_regioning pass which was
previously handled by lower_conversions, or whether it's a bug in the
lowering code of lower_regioning itself.
>>OPT(opt_copy_propagation);
>>OPT(dead_code_eliminate);
>>OPT(lower_simd_width);
>> diff --git a/src/intel/compiler/brw_fs.h
>> b/src/intel/compiler/brw_fs.h
>> index dc36ecc21ac..36825754931 100644
>> --- a/src/intel/compiler/brw_fs.h
>> +++ b/src/intel/compiler/brw_fs.h
>> @@ -164,6 +164,7 @@ public:
>> void lower_uniform_pull_constant_loads();
>> bool lower_load_payload();
>> bool lower_pack();
>> + bool lower_regioning();
>> bool lower_conversions();
>> bool lower_logical_sends();
>> bool lower_integer_multiplication();
>> @@ -536,24 +537,8 @@ namespace brw {
>>}
>> }
>>
>> - /**
>> -* Remove any modifiers from the \p i-th source region of the
>> instruction,
>> -* including negate, abs and any implicit type conversion to the
>> execution
>> -* type. Instead any source modifiers will be implemented as a
>> separate
>> -* MOV instruction prior to the original instruction.
>> -*/
>> - inline bool
>> - lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst
>> *inst, unsigned i)
>> - {
>> - assert(inst->components_read(i) == 1);
>> - const fs_builder ibld(v, block, inst);
>> - const fs_reg tmp = ibld.vgrf(get_exec_type(inst));
>> -
>> - ibld.MOV(tmp, inst->src[i]);
>> - inst->src[i] = tmp;
>> -
>> - return true;
>> - }
>> + bool
>> + lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst
>> *inst, unsigned i);
>> }
>>
>> void shuffle_from_32bit_read(const brw::fs_builder ,
>> diff --git a/src/intel/compiler/brw_fs_lower_regioning.cpp
>> b/src/intel/compiler/brw_fs_lower_regioning.cpp
>> new file mode 100644
>> index 000..d7c97e1442a
>> --- /dev/null
>> +++ b/src/intel/compiler/brw_fs_lower_regioning.cpp
>> @@ -0,0 +1,399 @@
>> +/*
>> + * Copyright © 2018 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without
Re: [Mesa-dev] [PATCHv2 07/10] intel/fs: Introduce regioning lowering pass.
On Sat, 2019-01-05 at 14:03 -0800, Francisco Jerez wrote:
> This legalization pass is meant to handle situations where the source
> or destination regioning controls of an instruction are unsupported
> by
> the hardware and need to be lowered away into separate instructions.
> This should be more reliable and future-proof than the current
> approach of handling CHV/BXT restrictions manually all over the
> visitor. The same mechanism is leveraged to lower unsupported type
> conversions easily, which obsoletes the lower_conversions pass.
>
> v2: Give conditional modifiers the same treatment as predicates for
> SEL instructions in lower_dst_modifiers() (Iago). Special-case a
> couple of other instructions with inconsistent conditional mod
> semantics in lower_dst_modifiers() (Curro).
> ---
> src/intel/Makefile.sources| 1 +
> src/intel/compiler/brw_fs.cpp | 5 +-
> src/intel/compiler/brw_fs.h | 21 +-
> src/intel/compiler/brw_fs_lower_regioning.cpp | 399
> ++
> src/intel/compiler/brw_ir_fs.h| 10 +
> src/intel/compiler/meson.build| 1 +
> 6 files changed, 418 insertions(+), 19 deletions(-)
> create mode 100644 src/intel/compiler/brw_fs_lower_regioning.cpp
>
> diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
> index 5e7d32293b7..6b9874d2b80 100644
> --- a/src/intel/Makefile.sources
> +++ b/src/intel/Makefile.sources
> @@ -64,6 +64,7 @@ COMPILER_FILES = \
> compiler/brw_fs_live_variables.h \
> compiler/brw_fs_lower_conversions.cpp \
> compiler/brw_fs_lower_pack.cpp \
> + compiler/brw_fs_lower_regioning.cpp \
> compiler/brw_fs_nir.cpp \
> compiler/brw_fs_reg_allocate.cpp \
> compiler/brw_fs_register_coalesce.cpp \
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index 889509badab..caa7a798332 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -6471,7 +6471,10 @@ fs_visitor::optimize()
>OPT(dead_code_eliminate);
> }
>
> - if (OPT(lower_conversions)) {
> + progress = false;
> + OPT(lower_conversions);
> + OPT(lower_regioning);
> + if (progress) {
This is a small nitpick but since this makes lower_conversions
redundant, maybe it makes more sense to just remove the call to it here
already in this patch so you don't have to reset the progress variable
and simply do:
if (OPT(lower_regioning)) {
...
}
>OPT(opt_copy_propagation);
>OPT(dead_code_eliminate);
>OPT(lower_simd_width);
> diff --git a/src/intel/compiler/brw_fs.h
> b/src/intel/compiler/brw_fs.h
> index dc36ecc21ac..36825754931 100644
> --- a/src/intel/compiler/brw_fs.h
> +++ b/src/intel/compiler/brw_fs.h
> @@ -164,6 +164,7 @@ public:
> void lower_uniform_pull_constant_loads();
> bool lower_load_payload();
> bool lower_pack();
> + bool lower_regioning();
> bool lower_conversions();
> bool lower_logical_sends();
> bool lower_integer_multiplication();
> @@ -536,24 +537,8 @@ namespace brw {
>}
> }
>
> - /**
> -* Remove any modifiers from the \p i-th source region of the
> instruction,
> -* including negate, abs and any implicit type conversion to the
> execution
> -* type. Instead any source modifiers will be implemented as a
> separate
> -* MOV instruction prior to the original instruction.
> -*/
> - inline bool
> - lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst
> *inst, unsigned i)
> - {
> - assert(inst->components_read(i) == 1);
> - const fs_builder ibld(v, block, inst);
> - const fs_reg tmp = ibld.vgrf(get_exec_type(inst));
> -
> - ibld.MOV(tmp, inst->src[i]);
> - inst->src[i] = tmp;
> -
> - return true;
> - }
> + bool
> + lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst
> *inst, unsigned i);
> }
>
> void shuffle_from_32bit_read(const brw::fs_builder ,
> diff --git a/src/intel/compiler/brw_fs_lower_regioning.cpp
> b/src/intel/compiler/brw_fs_lower_regioning.cpp
> new file mode 100644
> index 000..d7c97e1442a
> --- /dev/null
> +++ b/src/intel/compiler/brw_fs_lower_regioning.cpp
> @@ -0,0 +1,399 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice (including
> the next
> + * paragraph) shall be included in all copies or substantial
> portions of the
> + * Software.
> + *
> + *
[Mesa-dev] [PATCHv2 07/10] intel/fs: Introduce regioning lowering pass.
This legalization pass is meant to handle situations where the source
or destination regioning controls of an instruction are unsupported by
the hardware and need to be lowered away into separate instructions.
This should be more reliable and future-proof than the current
approach of handling CHV/BXT restrictions manually all over the
visitor. The same mechanism is leveraged to lower unsupported type
conversions easily, which obsoletes the lower_conversions pass.
v2: Give conditional modifiers the same treatment as predicates for
SEL instructions in lower_dst_modifiers() (Iago). Special-case a
couple of other instructions with inconsistent conditional mod
semantics in lower_dst_modifiers() (Curro).
---
src/intel/Makefile.sources| 1 +
src/intel/compiler/brw_fs.cpp | 5 +-
src/intel/compiler/brw_fs.h | 21 +-
src/intel/compiler/brw_fs_lower_regioning.cpp | 399 ++
src/intel/compiler/brw_ir_fs.h| 10 +
src/intel/compiler/meson.build| 1 +
6 files changed, 418 insertions(+), 19 deletions(-)
create mode 100644 src/intel/compiler/brw_fs_lower_regioning.cpp
diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
index 5e7d32293b7..6b9874d2b80 100644
--- a/src/intel/Makefile.sources
+++ b/src/intel/Makefile.sources
@@ -64,6 +64,7 @@ COMPILER_FILES = \
compiler/brw_fs_live_variables.h \
compiler/brw_fs_lower_conversions.cpp \
compiler/brw_fs_lower_pack.cpp \
+ compiler/brw_fs_lower_regioning.cpp \
compiler/brw_fs_nir.cpp \
compiler/brw_fs_reg_allocate.cpp \
compiler/brw_fs_register_coalesce.cpp \
diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
index 889509badab..caa7a798332 100644
--- a/src/intel/compiler/brw_fs.cpp
+++ b/src/intel/compiler/brw_fs.cpp
@@ -6471,7 +6471,10 @@ fs_visitor::optimize()
OPT(dead_code_eliminate);
}
- if (OPT(lower_conversions)) {
+ progress = false;
+ OPT(lower_conversions);
+ OPT(lower_regioning);
+ if (progress) {
OPT(opt_copy_propagation);
OPT(dead_code_eliminate);
OPT(lower_simd_width);
diff --git a/src/intel/compiler/brw_fs.h b/src/intel/compiler/brw_fs.h
index dc36ecc21ac..36825754931 100644
--- a/src/intel/compiler/brw_fs.h
+++ b/src/intel/compiler/brw_fs.h
@@ -164,6 +164,7 @@ public:
void lower_uniform_pull_constant_loads();
bool lower_load_payload();
bool lower_pack();
+ bool lower_regioning();
bool lower_conversions();
bool lower_logical_sends();
bool lower_integer_multiplication();
@@ -536,24 +537,8 @@ namespace brw {
}
}
- /**
-* Remove any modifiers from the \p i-th source region of the instruction,
-* including negate, abs and any implicit type conversion to the execution
-* type. Instead any source modifiers will be implemented as a separate
-* MOV instruction prior to the original instruction.
-*/
- inline bool
- lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst *inst, unsigned
i)
- {
- assert(inst->components_read(i) == 1);
- const fs_builder ibld(v, block, inst);
- const fs_reg tmp = ibld.vgrf(get_exec_type(inst));
-
- ibld.MOV(tmp, inst->src[i]);
- inst->src[i] = tmp;
-
- return true;
- }
+ bool
+ lower_src_modifiers(fs_visitor *v, bblock_t *block, fs_inst *inst, unsigned
i);
}
void shuffle_from_32bit_read(const brw::fs_builder ,
diff --git a/src/intel/compiler/brw_fs_lower_regioning.cpp
b/src/intel/compiler/brw_fs_lower_regioning.cpp
new file mode 100644
index 000..d7c97e1442a
--- /dev/null
+++ b/src/intel/compiler/brw_fs_lower_regioning.cpp
@@ -0,0 +1,399 @@
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "brw_fs.h"
+#include "brw_cfg.h"
+#include
