[08/10] intel/fs: Remove existing lower_conversions pass.

Submitted by Francisco Jerez on Dec. 29, 2018, 8:39 p.m.

Details

Message ID 20181229203904.18847-9-currojerez@riseup.net
State New
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Francisco Jerez Dec. 29, 2018, 8:39 p.m.
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

Patch hide | download patch | download mbox

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..00000000000
--- 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.
-             *
-             * So we need to allocate a temporary that's two registers, and then do
-             * a strided MOV to get the lower DWord of every Qword that has the
-             * result.
-             *
-             * This restriction applies, in general, whenever we convert to
-             * a type with a smaller bit-size.
-             */
-            fs_reg temp = ibld.vgrf(get_exec_type(inst));
-            fs_reg strided_temp = subscript(temp, dst.type, 0);
-
-            assert(inst->size_written == inst->dst.component_size(inst->exec_size));
-            inst->dst = strided_temp;
-            inst->saturate = false;
-            /* As it is an strided destination, we write n-times more being n the
-             * size ratio between source and destination types. Update
-             * size_written accordingly.
-             */
-            inst->size_written = inst->dst.component_size(inst->exec_size);
-
-            fs_inst *mov = ibld.at(block, inst->next).MOV(dst, strided_temp);
-            mov->saturate = saturate;
-            mov->predicate = inst->predicate;
-
-            progress = true;
-         }
-      } else {
-         fs_reg temp0 = ibld.vgrf(get_exec_type(inst));
-
-         assert(inst->size_written == inst->dst.component_size(inst->exec_size));
-         inst->dst = temp0;
-         /* As it is an strided destination, we write n-times more being n the
-          * size ratio between source and destination types. Update
-          * size_written accordingly.
-          */
-         inst->size_written = inst->dst.component_size(inst->exec_size);
-         inst->saturate = false;
-         /* Now, do the conversion to original destination's type. In next iteration,
-          * we will lower it if it is a d2f conversion.
-          */
-         ibld.at(block, inst->next).MOV(dst, temp0)->saturate = saturate;
-
-         progress = true;
-      }
-   }
-
-   if (progress)
-      invalidate_live_intervals();
-
-   return progress;
-}
diff --git a/src/intel/compiler/meson.build b/src/intel/compiler/meson.build
index 4af134b418e..f8e5e2518fe 100644
--- a/src/intel/compiler/meson.build
+++ b/src/intel/compiler/meson.build
@@ -55,7 +55,6 @@  libintel_compiler_files = files(
   'brw_fs.h',
   'brw_fs_live_variables.cpp',
   'brw_fs_live_variables.h',
-  'brw_fs_lower_conversions.cpp',
   'brw_fs_lower_pack.cpp',
   'brw_fs_lower_regioning.cpp',
   'brw_fs_nir.cpp',

Comments

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..00000000000
> --- 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.
> -             *
> -             * So we need to allocate a temporary that's two
> registers, and then do
> -             * a strided MOV to get the lower DWord of every Qword
> that has the
> -             * result.
> -             *
> -             * This restriction applies, in general, whenever we
> convert to
> -             * a type with a smaller bit-size.
> -             */
> -            fs_reg temp = ibld.vgrf(get_exec_type(inst));
> -            fs_reg strided_temp = subscript(temp, dst.type, 0);
> -
> -            assert(inst->size_written == inst-
> >dst.component_size(inst->exec_size));
> -            inst->dst = strided_temp;
> -            inst->saturate = false;
> -            /* As it is an strided destination, we write n-times
> more being n the
> -             * size ratio between source and destination types.
> Update
> -             * size_written accordingly.
> -             */
> -            inst->size_written = inst->dst.component_size(inst-
> >exec_size);
> -
> -            fs_inst *mov = ibld.at(block, inst->next).MOV(dst,
> strided_temp);
> -            mov->saturate = saturate;
> -            mov->predicate = inst->predicate;
> -
> -            progress = true;
> -         }
> -      } else {
> -         fs_reg temp0 = ibld.vgrf(get_exec_type(inst));
> -
> -         assert(inst->size_written == inst->dst.component_size(inst-
> >exec_size));
> -         inst->dst = temp0;
> -         /* As it is an strided destination, we write n-times more
> being n the
> -          * size ratio between source and destination types. Update
> -          * size_written accordingly.
> -          */
> -         inst->size_written = inst->dst.component_size(inst-
> >exec_size);
> -         inst->saturate = false;
> -         /* Now, do the conversion to original destination's type.
> In next iteration,
> -          * we will lower it if it is a d2f conversion.
> -          */
> -         ibld.at(block, inst->next).MOV(dst, temp0)->saturate =
> saturate;
> -
> -         progress = true;
> -      }
> -   }
> -
> -   if (progress)
> -      invalidate_live_intervals();
> -
> -   return progress;
> -}
> diff --git a/src/intel/compiler/meson.build
> b/src/intel/compiler/meson.build
> index 4af134b418e..f8e5e2518fe 100644
> --- a/src/intel/compiler/meson.build
> +++ b/src/intel/compiler/meson.build
> @@ -55,7 +55,6 @@ libintel_compiler_files = files(
>    'brw_fs.h',
>    'brw_fs_live_variables.cpp',
>    'brw_fs_live_variables.h',
> -  'brw_fs_lower_conversions.cpp',
>    'brw_fs_lower_pack.cpp',
>    'brw_fs_lower_regioning.cpp',
>    'brw_fs_nir.cpp',
Iago Toral <itoral@igalia.com> 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..00000000000
>> --- 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.
>> -             *
>> -             * So we need to allocate a temporary that's two
>> registers, and then do
>> -             * a strided MOV to get the lower DWord of every Qword
>> that has the
>> -             * result.
>> -             *
>> -             * This restriction applies, in general, whenever we
>> convert to
>> -             * a type with a smaller bit-size.
>> -             */
>> -            fs_reg temp = ibld.vgrf(get_exec_type(inst));
>> -            fs_reg strided_temp = subscript(temp, dst.type, 0);
>> -
>> -            assert(inst->size_written == inst-
>> >dst.component_size(inst->exec_size));
>> -            inst->dst = strided_temp;
>> -            inst->saturate = false;
>> -            /* As it is an strided destination, we write n-times
>> more being n the
>> -             * size ratio between source and destination types.
>> Update
>> -             * size_written accordingly.
>> -             */
>> -            inst->size_written = inst->dst.component_size(inst-
>> >exec_size);
>> -
>> -            fs_inst *mov = ibld.at(block, inst->next).MOV(dst,
>> strided_temp);
>> -            mov->saturate = saturate;
>> -            mov->predicate = inst->predicate;
>> -
>> -            progress = true;
>> -         }
>> -      } else {
>> -         fs_reg temp0 = ibld.vgrf(get_exec_type(inst));
>> -
>> -         assert(inst->size_written == inst->dst.component_size(inst-
>> >exec_size));
>> -         inst->dst = temp0;
>> -         /* As it is an strided destination, we write n-times more
>> being n the
>> -          * size ratio between source and destination types. Update
>> -          * size_written accordingly.
>> -          */
>> -         inst->size_written = inst->dst.component_size(inst-
>> >exec_size);
>> -         inst->saturate = false;
>> -         /* Now, do the conversion to original destination's type.
>> In next iteration,
>> -          * we will lower it if it is a d2f conversion.
>> -          */
>> -         ibld.at(block, inst->next).MOV(dst, temp0)->saturate =
>> saturate;
>> -
>> -         progress = true;
>> -      }
>> -   }
>> -
>> -   if (progress)
>> -      invalidate_live_intervals();
>> -
>> -   return progress;
>> -}
>> diff --git a/src/intel/compiler/meson.build
>> b/src/intel/compiler/meson.build
>> index 4af134b418e..f8e5e2518fe 100644
>> --- a/src/intel/compiler/meson.build
>> +++ b/src/intel/compiler/meson.build
>> @@ -55,7 +55,6 @@ libintel_compiler_files = files(
>>    'brw_fs.h',
>>    'brw_fs_live_variables.cpp',
>>    'brw_fs_live_variables.h',
>> -  'brw_fs_lower_conversions.cpp',
>>    'brw_fs_lower_pack.cpp',
>>    'brw_fs_lower_regioning.cpp',
>>    'brw_fs_nir.cpp',
On Mon, 2019-01-07 at 12:02 -0800, Francisco Jerez wrote:
> Iago Toral <itoral@igalia.com> 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..00000000000
> > > --- 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.
> > > -             *
> > > -             * So we need to allocate a temporary that's two
> > > registers, and then do
> > > -             * a strided MOV to get the lower DWord of every
> > > Qword
> > > that has the
> > > -             * result.
> > > -             *
> > > -             * This restriction applies, in general, whenever we
> > > convert to
> > > -             * a type with a smaller bit-size.
> > > -             */
> > > -            fs_reg temp = ibld.vgrf(get_exec_type(inst));
> > > -            fs_reg strided_temp = subscript(temp, dst.type, 0);
> > > -
> > > -            assert(inst->size_written == inst-
> > > > dst.component_size(inst->exec_size));
> > > 
> > > -            inst->dst = strided_temp;
> > > -            inst->saturate = false;
> > > -            /* As it is an strided destination, we write n-times
> > > more being n the
> > > -             * size ratio between source and destination types.
> > > Update
> > > -             * size_written accordingly.
> > > -             */
> > > -            inst->size_written = inst->dst.component_size(inst-
> > > > exec_size);
> > > 
> > > -
> > > -            fs_inst *mov = ibld.at(block, inst->next).MOV(dst,
> > > strided_temp);
> > > -            mov->saturate = saturate;
> > > -            mov->predicate = inst->predicate;
> > > -
> > > -            progress = true;
> > > -         }
> > > -      } else {
> > > -         fs_reg temp0 = ibld.vgrf(get_exec_type(inst));
> > > -
> > > -         assert(inst->size_written == inst-
> > > >dst.component_size(inst-
> > > > exec_size));
> > > 
> > > -         inst->dst = temp0;
> > > -         /* As it is an strided destination, we write n-times
> > > more
> > > being n the
> > > -          * size ratio between source and destination types.
> > > Update
> > > -          * size_written accordingly.
> > > -          */
> > > -         inst->size_written = inst->dst.component_size(inst-
> > > > exec_size);
> > > 
> > > -         inst->saturate = false;
> > > -         /* Now, do the conversion to original destination's
> > > type.
> > > In next iteration,
> > > -          * we will lower it if it is a d2f conversion.
> > > -          */
> > > -         ibld.at(block, inst->next).MOV(dst, temp0)->saturate =
> > > saturate;
> > > -
> > > -         progress = true;
> > > -      }
> > > -   }
> > > -
> > > -   if (progress)
> > > -      invalidate_live_intervals();
> > > -
> > > -   return progress;
> > > -}
> > > diff --git a/src/intel/compiler/meson.build
> > > b/src/intel/compiler/meson.build
> > > index 4af134b418e..f8e5e2518fe 100644
> > > --- a/src/intel/compiler/meson.build
> > > +++ b/src/intel/compiler/meson.build
> > > @@ -55,7 +55,6 @@ libintel_compiler_files = files(
> > >    'brw_fs.h',
> > >    'brw_fs_live_variables.cpp',
> > >    'brw_fs_live_variables.h',
> > > -  'brw_fs_lower_conversions.cpp',
> > >    'brw_fs_lower_pack.cpp',
> > >    'brw_fs_lower_regioning.cpp',
> > >    'brw_fs_nir.cpp',