[Mesa-dev,1/4] nir/instr_set: Add an allow_loads field

Submitted by Iago Toral Quiroga on Oct. 27, 2015, 9:28 a.m.

Details

Message ID 1445938141-28845-2-git-send-email-itoral@igalia.com
State New
Headers show
Series "Nir: implement a load-combine pass" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Oct. 27, 2015, 9:28 a.m.
We need this so we can configure different behaviors for passes that
cannot deal with side-effectful instructions (CSE) and passes that can
(we will add a load-combine pass shortly).

For now, when allow_loads is true, we let the instruction set rewrite
SSBO loads.
---
 src/glsl/nir/nir_instr_set.c | 51 ++++++++++++++++++++++++++++----------------
 src/glsl/nir/nir_instr_set.h | 20 ++++++++++++-----
 src/glsl/nir/nir_opt_cse.c   |  4 ++--
 3 files changed, 50 insertions(+), 25 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/glsl/nir/nir_instr_set.c b/src/glsl/nir/nir_instr_set.c
index d3f939f..583618f 100644
--- a/src/glsl/nir/nir_instr_set.c
+++ b/src/glsl/nir/nir_instr_set.c
@@ -398,6 +398,13 @@  dest_is_ssa(nir_dest *dest, void *data)
    return dest->is_ssa;
 }
 
+static bool
+is_load(nir_intrinsic_instr *instr)
+{
+   return instr->intrinsic == nir_intrinsic_load_ssbo ||
+          instr->intrinsic == nir_intrinsic_load_ssbo_indirect;
+}
+
 /* This function determines if uses of an instruction can safely be rewritten
  * to use another identical instruction instead. Note that this function must
  * be kept in sync with hash_instr() and nir_instrs_equal() -- only
@@ -406,7 +413,7 @@  dest_is_ssa(nir_dest *dest, void *data)
  */
 
 static bool
-instr_can_rewrite(nir_instr *instr)
+instr_can_rewrite(nir_instr *instr, bool allow_loads)
 {
    /* We only handle SSA. */
    if (!nir_foreach_dest(instr, dest_is_ssa, NULL) ||
@@ -428,11 +435,15 @@  instr_can_rewrite(nir_instr *instr)
       return true;
    }
    case nir_instr_type_intrinsic: {
+      nir_intrinsic_instr *intrinsic = nir_instr_as_intrinsic(instr);
       const nir_intrinsic_info *info =
-         &nir_intrinsic_infos[nir_instr_as_intrinsic(instr)->intrinsic];
-      return (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
-             (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
-             info->num_variables == 0; /* not implemented yet */
+         &nir_intrinsic_infos[intrinsic->intrinsic];
+      bool can_eliminate_and_reorder =
+         (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
+         (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
+         info->num_variables == 0; /* not implemented yet */
+      return can_eliminate_and_reorder ?
+         true: allow_loads && is_load(intrinsic);
    }
    case nir_instr_type_call:
    case nir_instr_type_jump:
@@ -475,25 +486,29 @@  cmp_func(const void *data1, const void *data2)
    return nir_instrs_equal(data1, data2);
 }
 
-struct set *
-nir_instr_set_create(void *mem_ctx)
+struct nir_instr_set *
+nir_instr_set_create(void *mem_ctx, bool allow_loads)
 {
-   return _mesa_set_create(mem_ctx, hash_instr, cmp_func);
+   struct nir_instr_set *instr_set = ralloc(mem_ctx, struct nir_instr_set);
+   instr_set->set = _mesa_set_create(mem_ctx, hash_instr, cmp_func);
+   instr_set->allow_loads = allow_loads;
+   return instr_set;
 }
 
 void
-nir_instr_set_destroy(struct set *instr_set)
+nir_instr_set_destroy(struct nir_instr_set *instr_set)
 {
-   _mesa_set_destroy(instr_set, NULL);
+   _mesa_set_destroy(instr_set->set, NULL);
+   ralloc_free(instr_set);
 }
 
 bool
-nir_instr_set_add_or_rewrite(struct set *instr_set, nir_instr *instr)
+nir_instr_set_add_or_rewrite(struct nir_instr_set *instr_set, nir_instr *instr)
 {
-   if (!instr_can_rewrite(instr))
+   if (!instr_can_rewrite(instr, instr_set->allow_loads))
       return false;
 
-   struct set_entry *entry = _mesa_set_search(instr_set, instr);
+   struct set_entry *entry = _mesa_set_search(instr_set->set, instr);
    if (entry) {
       nir_ssa_def *def = nir_instr_get_dest_ssa_def(instr);
       nir_ssa_def *new_def =
@@ -502,18 +517,18 @@  nir_instr_set_add_or_rewrite(struct set *instr_set, nir_instr *instr)
       return true;
    }
 
-   _mesa_set_add(instr_set, instr);
+   _mesa_set_add(instr_set->set, instr);
    return false;
 }
 
 void
-nir_instr_set_remove(struct set *instr_set, nir_instr *instr)
+nir_instr_set_remove(struct nir_instr_set *instr_set, nir_instr *instr)
 {
-   if (!instr_can_rewrite(instr))
+   if (!instr_can_rewrite(instr, instr_set->allow_loads))
       return;
 
-   struct set_entry *entry = _mesa_set_search(instr_set, instr);
+   struct set_entry *entry = _mesa_set_search(instr_set->set, instr);
    if (entry)
-      _mesa_set_remove(instr_set, entry);
+      _mesa_set_remove(instr_set->set, entry);
 }
 
diff --git a/src/glsl/nir/nir_instr_set.h b/src/glsl/nir/nir_instr_set.h
index 939e8dd..d00f9a8 100644
--- a/src/glsl/nir/nir_instr_set.h
+++ b/src/glsl/nir/nir_instr_set.h
@@ -38,11 +38,20 @@ 
 
 /*@{*/
 
-/** Creates an instruction set, using a given ralloc mem_ctx */
-struct set *nir_instr_set_create(void *mem_ctx);
+struct nir_instr_set {
+   struct set *set;
+   bool allow_loads;
+};
+
+/**
+ * Creates an instruction set, using a given ralloc mem_ctx. If allow_loads
+ * is true, then side-effectful instructions like SSBO loads that can't be
+ * freely moved around can still be rewritten.
+ */
+struct nir_instr_set *nir_instr_set_create(void *mem_ctx, bool allow_loads);
 
 /** Destroys an instruction set. */
-void nir_instr_set_destroy(struct set *instr_set);
+void nir_instr_set_destroy(struct nir_instr_set *instr_set);
 
 /**
  * Adds an instruction to an instruction set if it doesn't exist, or if it
@@ -50,13 +59,14 @@  void nir_instr_set_destroy(struct set *instr_set);
  * already-inserted instruction. Returns 'true' if the uses of the instruction
  * were rewritten.
  */
-bool nir_instr_set_add_or_rewrite(struct set *instr_set, nir_instr *instr);
+bool nir_instr_set_add_or_rewrite(struct nir_instr_set *instr_set,
+                                  nir_instr *instr);
 
 /**
  * Removes an instruction from an instruction set, so that other instructions
  * won't be merged with it.
  */
-void nir_instr_set_remove(struct set *instr_set, nir_instr *instr);
+void nir_instr_set_remove(struct nir_instr_set *instr_set, nir_instr *instr);
 
 /*@}*/
 
diff --git a/src/glsl/nir/nir_opt_cse.c b/src/glsl/nir/nir_opt_cse.c
index 93a6635..eb7296a 100644
--- a/src/glsl/nir/nir_opt_cse.c
+++ b/src/glsl/nir/nir_opt_cse.c
@@ -39,7 +39,7 @@ 
  */
 
 static bool
-cse_block(nir_block *block, struct set *instr_set)
+cse_block(nir_block *block, struct nir_instr_set *instr_set)
 {
    bool progress = false;
 
@@ -64,7 +64,7 @@  cse_block(nir_block *block, struct set *instr_set)
 static bool
 nir_opt_cse_impl(nir_function_impl *impl)
 {
-   struct set *instr_set = nir_instr_set_create(NULL);
+   struct nir_instr_set *instr_set = nir_instr_set_create(NULL, false);
 
    nir_metadata_require(impl, nir_metadata_dominance);
 

Comments

On Tue, Oct 27, 2015 at 10:28:58AM +0100, Iago Toral Quiroga wrote:
> We need this so we can configure different behaviors for passes that
> cannot deal with side-effectful instructions (CSE) and passes that can
> (we will add a load-combine pass shortly).
> 
> For now, when allow_loads is true, we let the instruction set rewrite
> SSBO loads.
> ---
>  src/glsl/nir/nir_instr_set.c | 51 ++++++++++++++++++++++++++++----------------
>  src/glsl/nir/nir_instr_set.h | 20 ++++++++++++-----
>  src/glsl/nir/nir_opt_cse.c   |  4 ++--
>  3 files changed, 50 insertions(+), 25 deletions(-)
> 
> diff --git a/src/glsl/nir/nir_instr_set.c b/src/glsl/nir/nir_instr_set.c
> index d3f939f..583618f 100644
> --- a/src/glsl/nir/nir_instr_set.c
> +++ b/src/glsl/nir/nir_instr_set.c
> @@ -398,6 +398,13 @@ dest_is_ssa(nir_dest *dest, void *data)
>     return dest->is_ssa;
>  }
>  
> +static bool
> +is_load(nir_intrinsic_instr *instr)
> +{
> +   return instr->intrinsic == nir_intrinsic_load_ssbo ||
> +          instr->intrinsic == nir_intrinsic_load_ssbo_indirect;
> +}
> +
>  /* This function determines if uses of an instruction can safely be rewritten
>   * to use another identical instruction instead. Note that this function must
>   * be kept in sync with hash_instr() and nir_instrs_equal() -- only
> @@ -406,7 +413,7 @@ dest_is_ssa(nir_dest *dest, void *data)
>   */
>  
>  static bool
> -instr_can_rewrite(nir_instr *instr)
> +instr_can_rewrite(nir_instr *instr, bool allow_loads)
>  {
>     /* We only handle SSA. */
>     if (!nir_foreach_dest(instr, dest_is_ssa, NULL) ||
> @@ -428,11 +435,15 @@ instr_can_rewrite(nir_instr *instr)
>        return true;
>     }
>     case nir_instr_type_intrinsic: {
> +      nir_intrinsic_instr *intrinsic = nir_instr_as_intrinsic(instr);
>        const nir_intrinsic_info *info =
> -         &nir_intrinsic_infos[nir_instr_as_intrinsic(instr)->intrinsic];
> -      return (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
> -             (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
> -             info->num_variables == 0; /* not implemented yet */
> +         &nir_intrinsic_infos[intrinsic->intrinsic];
> +      bool can_eliminate_and_reorder =
> +         (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
> +         (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
> +         info->num_variables == 0; /* not implemented yet */
> +      return can_eliminate_and_reorder ?
> +         true: allow_loads && is_load(intrinsic);

Isn't this just?

         return can_eliminate_and_reorder ||
                (allow_loads && is_load(intrinsic));
On Tue, 2015-10-27 at 14:33 +0200, Pohjolainen, Topi wrote:
> On Tue, Oct 27, 2015 at 10:28:58AM +0100, Iago Toral Quiroga wrote:
> > We need this so we can configure different behaviors for passes that
> > cannot deal with side-effectful instructions (CSE) and passes that can
> > (we will add a load-combine pass shortly).
> > 
> > For now, when allow_loads is true, we let the instruction set rewrite
> > SSBO loads.
> > ---
> >  src/glsl/nir/nir_instr_set.c | 51 ++++++++++++++++++++++++++++----------------
> >  src/glsl/nir/nir_instr_set.h | 20 ++++++++++++-----
> >  src/glsl/nir/nir_opt_cse.c   |  4 ++--
> >  3 files changed, 50 insertions(+), 25 deletions(-)
> > 
> > diff --git a/src/glsl/nir/nir_instr_set.c b/src/glsl/nir/nir_instr_set.c
> > index d3f939f..583618f 100644
> > --- a/src/glsl/nir/nir_instr_set.c
> > +++ b/src/glsl/nir/nir_instr_set.c
> > @@ -398,6 +398,13 @@ dest_is_ssa(nir_dest *dest, void *data)
> >     return dest->is_ssa;
> >  }
> >  
> > +static bool
> > +is_load(nir_intrinsic_instr *instr)
> > +{
> > +   return instr->intrinsic == nir_intrinsic_load_ssbo ||
> > +          instr->intrinsic == nir_intrinsic_load_ssbo_indirect;
> > +}
> > +
> >  /* This function determines if uses of an instruction can safely be rewritten
> >   * to use another identical instruction instead. Note that this function must
> >   * be kept in sync with hash_instr() and nir_instrs_equal() -- only
> > @@ -406,7 +413,7 @@ dest_is_ssa(nir_dest *dest, void *data)
> >   */
> >  
> >  static bool
> > -instr_can_rewrite(nir_instr *instr)
> > +instr_can_rewrite(nir_instr *instr, bool allow_loads)
> >  {
> >     /* We only handle SSA. */
> >     if (!nir_foreach_dest(instr, dest_is_ssa, NULL) ||
> > @@ -428,11 +435,15 @@ instr_can_rewrite(nir_instr *instr)
> >        return true;
> >     }
> >     case nir_instr_type_intrinsic: {
> > +      nir_intrinsic_instr *intrinsic = nir_instr_as_intrinsic(instr);
> >        const nir_intrinsic_info *info =
> > -         &nir_intrinsic_infos[nir_instr_as_intrinsic(instr)->intrinsic];
> > -      return (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
> > -             (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
> > -             info->num_variables == 0; /* not implemented yet */
> > +         &nir_intrinsic_infos[intrinsic->intrinsic];
> > +      bool can_eliminate_and_reorder =
> > +         (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
> > +         (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
> > +         info->num_variables == 0; /* not implemented yet */
> > +      return can_eliminate_and_reorder ?
> > +         true: allow_loads && is_load(intrinsic);
> 
> Isn't this just?
> 
>          return can_eliminate_and_reorder ||
>                 (allow_loads && is_load(intrinsic));
> 
> Received: from fanzine.local.igalia.com ([192.168.10.13] helo=fanzine.igalia.com)
> 	by mail.igalia.com with esmtps 
> 	(Cipher TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim)
> 	id 1Zr3SP-0002rb-7Q
> 	for <itoral@igalia.com>; Tue, 27 Oct 2015 13:34:29 +0100
> Received: from mga14.intel.com ([192.55.52.115])
> 	by fanzine.igalia.com with esmtp (Exim)
> 	id 1Zr3SO-0001hB-Rd
> 	for <itoral@igalia.com>; Tue, 27 Oct 2015 13:34:29 +0100
> Received: from fmsmga002.fm.intel.com ([10.253.24.26])
>   by fmsmga103.fm.intel.com with ESMTP; 27 Oct 2015 05:33:51 -0700
> X-ExtLoop1: 1
> X-IronPort-AV: E=Sophos;i="5.20,205,1444719600"; 
>    d="scan'208";a="836522023"
> Received: from kgoijens-mobl5.ger.corp.intel.com (HELO nelli) ([10.252.24.134])
>   by fmsmga002.fm.intel.com with ESMTP; 27 Oct 2015 05:33:50 -0700
> Date: Tue, 27 Oct 2015 14:33:50 +0200
> From: "Pohjolainen, Topi" <topi.pohjolainen@intel.com>
> To: Iago Toral Quiroga <itoral@igalia.com>
> Cc: mesa-dev@lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH 1/4] nir/instr_set: Add an allow_loads field
> Message-ID: <20151027123349.GB2575@nelli.ger.corp.intel.com>
> References: <1445938141-28845-1-git-send-email-itoral@igalia.com>
>  <1445938141-28845-2-git-send-email-itoral@igalia.com>
> MIME-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> In-Reply-To: <1445938141-28845-2-git-send-email-itoral@igalia.com>
> User-Agent: Mutt/1.5.23 (2014-03-12)
> 
> On Tue, Oct 27, 2015 at 10:28:58AM +0100, Iago Toral Quiroga wrote:
> > We need this so we can configure different behaviors for passes that
> > cannot deal with side-effectful instructions (CSE) and passes that can
> > (we will add a load-combine pass shortly).
> > 
> > For now, when allow_loads is true, we let the instruction set rewrite
> > SSBO loads.
> > ---
> >  src/glsl/nir/nir_instr_set.c | 51 ++++++++++++++++++++++++++++----------------
> >  src/glsl/nir/nir_instr_set.h | 20 ++++++++++++-----
> >  src/glsl/nir/nir_opt_cse.c   |  4 ++--
> >  3 files changed, 50 insertions(+), 25 deletions(-)
> > 
> > diff --git a/src/glsl/nir/nir_instr_set.c b/src/glsl/nir/nir_instr_set.c
> > index d3f939f..583618f 100644
> > --- a/src/glsl/nir/nir_instr_set.c
> > +++ b/src/glsl/nir/nir_instr_set.c
> > @@ -398,6 +398,13 @@ dest_is_ssa(nir_dest *dest, void *data)
> >     return dest->is_ssa;
> >  }
> >  
> > +static bool
> > +is_load(nir_intrinsic_instr *instr)
> > +{
> > +   return instr->intrinsic == nir_intrinsic_load_ssbo ||
> > +          instr->intrinsic == nir_intrinsic_load_ssbo_indirect;
> > +}
> > +
> >  /* This function determines if uses of an instruction can safely be rewritten
> >   * to use another identical instruction instead. Note that this function must
> >   * be kept in sync with hash_instr() and nir_instrs_equal() -- only
> > @@ -406,7 +413,7 @@ dest_is_ssa(nir_dest *dest, void *data)
> >   */
> >  
> >  static bool
> > -instr_can_rewrite(nir_instr *instr)
> > +instr_can_rewrite(nir_instr *instr, bool allow_loads)
> >  {
> >     /* We only handle SSA. */
> >     if (!nir_foreach_dest(instr, dest_is_ssa, NULL) ||
> > @@ -428,11 +435,15 @@ instr_can_rewrite(nir_instr *instr)
> >        return true;
> >     }
> >     case nir_instr_type_intrinsic: {
> > +      nir_intrinsic_instr *intrinsic = nir_instr_as_intrinsic(instr);
> >        const nir_intrinsic_info *info =
> > -         &nir_intrinsic_infos[nir_instr_as_intrinsic(instr)->intrinsic];
> > -      return (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
> > -             (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
> > -             info->num_variables == 0; /* not implemented yet */
> > +         &nir_intrinsic_infos[intrinsic->intrinsic];
> > +      bool can_eliminate_and_reorder =
> > +         (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
> > +         (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
> > +         info->num_variables == 0; /* not implemented yet */
> > +      return can_eliminate_and_reorder ?
> > +         true: allow_loads && is_load(intrinsic);
> 
> Isn't this just?
> 
>          return can_eliminate_and_reorder ||
>                 (allow_loads && is_load(intrinsic));

Right, changed locally. Thanks!

Iago