[v3,22/42] intel/compiler: don't propagate HF immediates to 3-src instructions

Submitted by Iago Toral Quiroga on Jan. 15, 2019, 1:53 p.m.

Details

Message ID 20190115135414.2313-23-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 5 4 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Jan. 15, 2019, 1:53 p.m.
3-src instructions don't support immediates, but since 36bc5f06dd22,
we allow them on MAD and LRP relying on the combine constants pass to
fix it up later. However, that pass is specialized for 32-bit float
immediates and can't handle HF constants at present, so this patch
ensures that copy-propagation only does this for 32-bit constants.

Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
---
 src/intel/compiler/brw_fs_copy_propagation.cpp | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_fs_copy_propagation.cpp b/src/intel/compiler/brw_fs_copy_propagation.cpp
index c23ce1ef426..77f2749ba04 100644
--- a/src/intel/compiler/brw_fs_copy_propagation.cpp
+++ b/src/intel/compiler/brw_fs_copy_propagation.cpp
@@ -772,8 +772,16 @@  fs_visitor::try_constant_propagate(fs_inst *inst, acp_entry *entry)
 
       case BRW_OPCODE_MAD:
       case BRW_OPCODE_LRP:
-         inst->src[i] = val;
-         progress = true;
+         /* 3-src instructions can't take IMM registers, however, for 32-bit
+          * floating instructions we rely on the combine constants pass to fix
+          * it up. For anything else, we shouldn't be promoting immediates
+          * until we can make the pass capable of combining constants of
+          * different sizes.
+          */
+         if (val.type == BRW_REGISTER_TYPE_F) {
+            inst->src[i] = val;
+            progress = true;
+         }
          break;
 
       default:

Comments

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

On Tue, Jan 15, 2019 at 7:54 AM Iago Toral Quiroga <itoral@igalia.com>
wrote:

> 3-src instructions don't support immediates, but since 36bc5f06dd22,
> we allow them on MAD and LRP relying on the combine constants pass to
> fix it up later. However, that pass is specialized for 32-bit float
> immediates and can't handle HF constants at present, so this patch
> ensures that copy-propagation only does this for 32-bit constants.
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
> ---
>  src/intel/compiler/brw_fs_copy_propagation.cpp | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_copy_propagation.cpp
> b/src/intel/compiler/brw_fs_copy_propagation.cpp
> index c23ce1ef426..77f2749ba04 100644
> --- a/src/intel/compiler/brw_fs_copy_propagation.cpp
> +++ b/src/intel/compiler/brw_fs_copy_propagation.cpp
> @@ -772,8 +772,16 @@ fs_visitor::try_constant_propagate(fs_inst *inst,
> acp_entry *entry)
>
>        case BRW_OPCODE_MAD:
>        case BRW_OPCODE_LRP:
> -         inst->src[i] = val;
> -         progress = true;
> +         /* 3-src instructions can't take IMM registers, however, for
> 32-bit
> +          * floating instructions we rely on the combine constants pass
> to fix
> +          * it up. For anything else, we shouldn't be promoting immediates
> +          * until we can make the pass capable of combining constants of
> +          * different sizes.
> +          */
> +         if (val.type == BRW_REGISTER_TYPE_F) {
> +            inst->src[i] = val;
> +            progress = true;
> +         }
>           break;
>
>        default:
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Tue, Jan 15, 2019 at 5:54 AM Iago Toral Quiroga <itoral@igalia.com> wrote:
>
> 3-src instructions don't support immediates, but since 36bc5f06dd22,
> we allow them on MAD and LRP relying on the combine constants pass to
> fix it up later. However, that pass is specialized for 32-bit float
> immediates and can't handle HF constants at present, so this patch
> ensures that copy-propagation only does this for 32-bit constants.

There's a patch later in the series that adds HF support to constant
combining (and presumably removes this code). Maybe it's the best
thing to add and remove the code in the same series, but it's good to
at least mention that in the commit message so that reviewers
understand what the plan is.

I see from a later thread that you're going to try to handle more than
just F/HF types in constant combining, and I guess you'll resend this
series. If that's the case, I'd just leave this patch out if it's
possible to reorder things.

Oh, another thing: Gen10+ can take *1* immediate HF argument in 3-src
instructions. We haven't added that support yet, though all the
low-level brw_inst_* functions exist. Not asking you to do that
without hardware, but just thought you'd be interested to know :)
On Tue, 2019-01-22 at 16:18 -0800, Matt Turner wrote:
> On Tue, Jan 15, 2019 at 5:54 AM Iago Toral Quiroga <itoral@igalia.com
> > wrote:
> > 
> > 3-src instructions don't support immediates, but since
> > 36bc5f06dd22,
> > we allow them on MAD and LRP relying on the combine constants pass
> > to
> > fix it up later. However, that pass is specialized for 32-bit float
> > immediates and can't handle HF constants at present, so this patch
> > ensures that copy-propagation only does this for 32-bit constants.
> 
> There's a patch later in the series that adds HF support to constant
> combining (and presumably removes this code). Maybe it's the best
> thing to add and remove the code in the same series, but it's good to
> at least mention that in the commit message so that reviewers
> understand what the plan is.

Yes, makes sense.

> I see from a later thread that you're going to try to handle more
> than
> just F/HF types in constant combining, and I guess you'll resend this
> series. If that's the case, I'd just leave this patch out if it's
> possible to reorder things.

Yes, I'll do that.

> Oh, another thing: Gen10+ can take *1* immediate HF argument in 3-src
> instructions. We haven't added that support yet, though all the
> low-level brw_inst_* functions exist. Not asking you to do that
> without hardware, but just thought you'd be interested to know :)

Sure, that's good to know, thanks for the info! :)