Re: [Mesa-dev] [PATCH 08/10] intel/fs: Remove existing lower_conversions pass.
On Mon, 2019-01-07 at 12:02 -0800, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Sat, 2018-12-29 at 12:39 -0800, Francisco Jerez wrote:
> > > It's redundant with the functionality provided by lower_regioning
> > > now.
> > > ---
> > > src/intel/Makefile.sources| 1 -
> > > src/intel/compiler/brw_fs.cpp | 1 -
> > > src/intel/compiler/brw_fs.h | 1 -
> > > .../compiler/brw_fs_lower_conversions.cpp | 132
> > >
> > > --
> > > src/intel/compiler/meson.build| 1 -
> > > 5 files changed, 136 deletions(-)
> > > delete mode 100644
> > > src/intel/compiler/brw_fs_lower_conversions.cpp
> > >
> > > diff --git a/src/intel/Makefile.sources
> > > b/src/intel/Makefile.sources
> > > index 6b9874d2b80..fe06a57b42e 100644
> > > --- a/src/intel/Makefile.sources
> > > +++ b/src/intel/Makefile.sources
> > > @@ -62,7 +62,6 @@ COMPILER_FILES = \
> > > compiler/brw_fs.h \
> > > compiler/brw_fs_live_variables.cpp \
> > > 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 \
> > > diff --git a/src/intel/compiler/brw_fs.cpp
> > > b/src/intel/compiler/brw_fs.cpp
> > > index caa7a798332..d6280d558ec 100644
> > > --- a/src/intel/compiler/brw_fs.cpp
> > > +++ b/src/intel/compiler/brw_fs.cpp
> > > @@ -6472,7 +6472,6 @@ fs_visitor::optimize()
> > > }
> > >
> > > progress = false;
> > > - OPT(lower_conversions);
> > > OPT(lower_regioning);
> > > if (progress) {
> > >OPT(opt_copy_propagation);
> >
> > If you didn't do this in the previous patch, then maybe do it here:
> >
> > if (OPT(lower_regioning)) {
> >...
> > }
> >
> > and avoid resetting progress.
> >
>
> I left this lying around because there is another legalization pass
> coming up that should cause the same post-lowering optimization
> passes
> to be executed if progress is made. I can clean things up though if
> you
> like, and re-introduce the reset of the progress flag in the future
> commit.
If we are still going to need it soon I think it is fine to leave it
like this.
> > > diff --git a/src/intel/compiler/brw_fs.h
> > > b/src/intel/compiler/brw_fs.h
> > > index 36825754931..7edaa3af43c 100644
> > > --- a/src/intel/compiler/brw_fs.h
> > > +++ b/src/intel/compiler/brw_fs.h
> > > @@ -165,7 +165,6 @@ public:
> > > bool lower_load_payload();
> > > bool lower_pack();
> > > bool lower_regioning();
> > > - bool lower_conversions();
> > > bool lower_logical_sends();
> > > bool lower_integer_multiplication();
> > > bool lower_minmax();
> > > diff --git a/src/intel/compiler/brw_fs_lower_conversions.cpp
> > > b/src/intel/compiler/brw_fs_lower_conversions.cpp
> > > deleted file mode 100644
> > > index 145fb55f995..000
> > > --- a/src/intel/compiler/brw_fs_lower_conversions.cpp
> > > +++ /dev/null
> > > @@ -1,132 +0,0 @@
> > > -/*
> > > - * Copyright © 2015 Connor Abbott
> > > - *
> > > - * 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 "brw_fs_builder.h"
> > > -
> > > -using namespace brw;
> > > -
> > > -static bool
> > > -supports_type_conversion(const fs_inst *inst) {
> > > - switch (inst->opcode) {
> > > - case BRW_OPCODE_MOV:
> > > - case SHADER_OPCODE_MOV_INDIRECT:
> > > - return true;
> > > - case BRW_OPCODE_SEL:
> > > - return inst->dst.type == get_exec_type(inst);
> > > - default:
> > > - /* FIXME: We assume the opcodes don't explicitly mentioned
> > > -
Re: [Mesa-dev] [PATCH 08/10] intel/fs: Remove existing lower_conversions pass.
Iago Toral writes:
> On Sat, 2018-12-29 at 12:39 -0800, Francisco Jerez wrote:
>> It's redundant with the functionality provided by lower_regioning
>> now.
>> ---
>> src/intel/Makefile.sources| 1 -
>> src/intel/compiler/brw_fs.cpp | 1 -
>> src/intel/compiler/brw_fs.h | 1 -
>> .../compiler/brw_fs_lower_conversions.cpp | 132
>> --
>> src/intel/compiler/meson.build| 1 -
>> 5 files changed, 136 deletions(-)
>> delete mode 100644 src/intel/compiler/brw_fs_lower_conversions.cpp
>>
>> diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
>> index 6b9874d2b80..fe06a57b42e 100644
>> --- a/src/intel/Makefile.sources
>> +++ b/src/intel/Makefile.sources
>> @@ -62,7 +62,6 @@ COMPILER_FILES = \
>> compiler/brw_fs.h \
>> compiler/brw_fs_live_variables.cpp \
>> 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 \
>> diff --git a/src/intel/compiler/brw_fs.cpp
>> b/src/intel/compiler/brw_fs.cpp
>> index caa7a798332..d6280d558ec 100644
>> --- a/src/intel/compiler/brw_fs.cpp
>> +++ b/src/intel/compiler/brw_fs.cpp
>> @@ -6472,7 +6472,6 @@ fs_visitor::optimize()
>> }
>>
>> progress = false;
>> - OPT(lower_conversions);
>> OPT(lower_regioning);
>> if (progress) {
>>OPT(opt_copy_propagation);
>
> If you didn't do this in the previous patch, then maybe do it here:
>
> if (OPT(lower_regioning)) {
>...
> }
>
> and avoid resetting progress.
>
I left this lying around because there is another legalization pass
coming up that should cause the same post-lowering optimization passes
to be executed if progress is made. I can clean things up though if you
like, and re-introduce the reset of the progress flag in the future
commit.
>> diff --git a/src/intel/compiler/brw_fs.h
>> b/src/intel/compiler/brw_fs.h
>> index 36825754931..7edaa3af43c 100644
>> --- a/src/intel/compiler/brw_fs.h
>> +++ b/src/intel/compiler/brw_fs.h
>> @@ -165,7 +165,6 @@ public:
>> bool lower_load_payload();
>> bool lower_pack();
>> bool lower_regioning();
>> - bool lower_conversions();
>> bool lower_logical_sends();
>> bool lower_integer_multiplication();
>> bool lower_minmax();
>> diff --git a/src/intel/compiler/brw_fs_lower_conversions.cpp
>> b/src/intel/compiler/brw_fs_lower_conversions.cpp
>> deleted file mode 100644
>> index 145fb55f995..000
>> --- a/src/intel/compiler/brw_fs_lower_conversions.cpp
>> +++ /dev/null
>> @@ -1,132 +0,0 @@
>> -/*
>> - * Copyright © 2015 Connor Abbott
>> - *
>> - * 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 "brw_fs_builder.h"
>> -
>> -using namespace brw;
>> -
>> -static bool
>> -supports_type_conversion(const fs_inst *inst) {
>> - switch (inst->opcode) {
>> - case BRW_OPCODE_MOV:
>> - case SHADER_OPCODE_MOV_INDIRECT:
>> - return true;
>> - case BRW_OPCODE_SEL:
>> - return inst->dst.type == get_exec_type(inst);
>> - default:
>> - /* FIXME: We assume the opcodes don't explicitly mentioned
>> - * before just work fine with arbitrary conversions.
>> - */
>> - return true;
>> - }
>> -}
>> -
>> -/* From the SKL PRM Vol 2a, "Move":
>> - *
>> - *"A mov with the same source and destination type, no source
>> modifier,
>> - * and no saturation is a raw move. A packed byte destination
>> region (B
>> - * or UB type with HorzStride == 1 and ExecSize > 1) can only be
>> written
>> - * using raw move."
>> - */
>> -static bool
>> -is_byte_raw_mov (const fs_inst *inst)
>> -{
>> - return type_sz(inst->dst.type) == 1
Re: [Mesa-dev] [PATCH 08/10] intel/fs: Remove existing lower_conversions pass.
On Sat, 2018-12-29 at 12:39 -0800, Francisco Jerez wrote:
> It's redundant with the functionality provided by lower_regioning
> now.
> ---
> src/intel/Makefile.sources| 1 -
> src/intel/compiler/brw_fs.cpp | 1 -
> src/intel/compiler/brw_fs.h | 1 -
> .../compiler/brw_fs_lower_conversions.cpp | 132
> --
> src/intel/compiler/meson.build| 1 -
> 5 files changed, 136 deletions(-)
> delete mode 100644 src/intel/compiler/brw_fs_lower_conversions.cpp
>
> diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
> index 6b9874d2b80..fe06a57b42e 100644
> --- a/src/intel/Makefile.sources
> +++ b/src/intel/Makefile.sources
> @@ -62,7 +62,6 @@ COMPILER_FILES = \
> compiler/brw_fs.h \
> compiler/brw_fs_live_variables.cpp \
> 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 \
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index caa7a798332..d6280d558ec 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -6472,7 +6472,6 @@ fs_visitor::optimize()
> }
>
> progress = false;
> - OPT(lower_conversions);
> OPT(lower_regioning);
> if (progress) {
>OPT(opt_copy_propagation);
If you didn't do this in the previous patch, then maybe do it here:
if (OPT(lower_regioning)) {
...
}
and avoid resetting progress.
> diff --git a/src/intel/compiler/brw_fs.h
> b/src/intel/compiler/brw_fs.h
> index 36825754931..7edaa3af43c 100644
> --- a/src/intel/compiler/brw_fs.h
> +++ b/src/intel/compiler/brw_fs.h
> @@ -165,7 +165,6 @@ public:
> bool lower_load_payload();
> bool lower_pack();
> bool lower_regioning();
> - bool lower_conversions();
> bool lower_logical_sends();
> bool lower_integer_multiplication();
> bool lower_minmax();
> diff --git a/src/intel/compiler/brw_fs_lower_conversions.cpp
> b/src/intel/compiler/brw_fs_lower_conversions.cpp
> deleted file mode 100644
> index 145fb55f995..000
> --- a/src/intel/compiler/brw_fs_lower_conversions.cpp
> +++ /dev/null
> @@ -1,132 +0,0 @@
> -/*
> - * Copyright © 2015 Connor Abbott
> - *
> - * 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 "brw_fs_builder.h"
> -
> -using namespace brw;
> -
> -static bool
> -supports_type_conversion(const fs_inst *inst) {
> - switch (inst->opcode) {
> - case BRW_OPCODE_MOV:
> - case SHADER_OPCODE_MOV_INDIRECT:
> - return true;
> - case BRW_OPCODE_SEL:
> - return inst->dst.type == get_exec_type(inst);
> - default:
> - /* FIXME: We assume the opcodes don't explicitly mentioned
> - * before just work fine with arbitrary conversions.
> - */
> - return true;
> - }
> -}
> -
> -/* From the SKL PRM Vol 2a, "Move":
> - *
> - *"A mov with the same source and destination type, no source
> modifier,
> - * and no saturation is a raw move. A packed byte destination
> region (B
> - * or UB type with HorzStride == 1 and ExecSize > 1) can only be
> written
> - * using raw move."
> - */
> -static bool
> -is_byte_raw_mov (const fs_inst *inst)
> -{
> - return type_sz(inst->dst.type) == 1 &&
> - inst->opcode == BRW_OPCODE_MOV &&
> - inst->src[0].type == inst->dst.type &&
> - !inst->saturate &&
> - !inst->src[0].negate &&
> - !inst->src[0].abs;
> -}
> -
> -bool
> -fs_visitor::lower_conversions()
> -{
> - bool progress = false;
> -
> - foreach_block_and_inst(block, fs_inst, inst, cfg) {
> - const fs_builder ibld(this, block, inst);
> - fs_reg dst = inst->dst;
> - bool
[Mesa-dev] [PATCH 08/10] intel/fs: Remove existing lower_conversions pass.
It's redundant with the functionality provided by lower_regioning now.
---
src/intel/Makefile.sources| 1 -
src/intel/compiler/brw_fs.cpp | 1 -
src/intel/compiler/brw_fs.h | 1 -
.../compiler/brw_fs_lower_conversions.cpp | 132 --
src/intel/compiler/meson.build| 1 -
5 files changed, 136 deletions(-)
delete mode 100644 src/intel/compiler/brw_fs_lower_conversions.cpp
diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
index 6b9874d2b80..fe06a57b42e 100644
--- a/src/intel/Makefile.sources
+++ b/src/intel/Makefile.sources
@@ -62,7 +62,6 @@ COMPILER_FILES = \
compiler/brw_fs.h \
compiler/brw_fs_live_variables.cpp \
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 \
diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
index caa7a798332..d6280d558ec 100644
--- a/src/intel/compiler/brw_fs.cpp
+++ b/src/intel/compiler/brw_fs.cpp
@@ -6472,7 +6472,6 @@ fs_visitor::optimize()
}
progress = false;
- OPT(lower_conversions);
OPT(lower_regioning);
if (progress) {
OPT(opt_copy_propagation);
diff --git a/src/intel/compiler/brw_fs.h b/src/intel/compiler/brw_fs.h
index 36825754931..7edaa3af43c 100644
--- a/src/intel/compiler/brw_fs.h
+++ b/src/intel/compiler/brw_fs.h
@@ -165,7 +165,6 @@ public:
bool lower_load_payload();
bool lower_pack();
bool lower_regioning();
- bool lower_conversions();
bool lower_logical_sends();
bool lower_integer_multiplication();
bool lower_minmax();
diff --git a/src/intel/compiler/brw_fs_lower_conversions.cpp
b/src/intel/compiler/brw_fs_lower_conversions.cpp
deleted file mode 100644
index 145fb55f995..000
--- a/src/intel/compiler/brw_fs_lower_conversions.cpp
+++ /dev/null
@@ -1,132 +0,0 @@
-/*
- * Copyright © 2015 Connor Abbott
- *
- * 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 "brw_fs_builder.h"
-
-using namespace brw;
-
-static bool
-supports_type_conversion(const fs_inst *inst) {
- switch (inst->opcode) {
- case BRW_OPCODE_MOV:
- case SHADER_OPCODE_MOV_INDIRECT:
- return true;
- case BRW_OPCODE_SEL:
- return inst->dst.type == get_exec_type(inst);
- default:
- /* FIXME: We assume the opcodes don't explicitly mentioned
- * before just work fine with arbitrary conversions.
- */
- return true;
- }
-}
-
-/* From the SKL PRM Vol 2a, "Move":
- *
- *"A mov with the same source and destination type, no source modifier,
- * and no saturation is a raw move. A packed byte destination region (B
- * or UB type with HorzStride == 1 and ExecSize > 1) can only be written
- * using raw move."
- */
-static bool
-is_byte_raw_mov (const fs_inst *inst)
-{
- return type_sz(inst->dst.type) == 1 &&
- inst->opcode == BRW_OPCODE_MOV &&
- inst->src[0].type == inst->dst.type &&
- !inst->saturate &&
- !inst->src[0].negate &&
- !inst->src[0].abs;
-}
-
-bool
-fs_visitor::lower_conversions()
-{
- bool progress = false;
-
- foreach_block_and_inst(block, fs_inst, inst, cfg) {
- const fs_builder ibld(this, block, inst);
- fs_reg dst = inst->dst;
- bool saturate = inst->saturate;
-
- if (supports_type_conversion(inst)) {
- if (type_sz(inst->dst.type) < get_exec_type_size(inst) &&
- !is_byte_raw_mov(inst)) {
-/* From the Broadwell PRM, 3D Media GPGPU, "Double Precision Float
to
- * Single Precision Float":
- *
- *The upper Dword of every Qword will be written with undefined
- *value when converting DF to F.
- *
-
