Re: [Mesa-dev] [PATCH v3 23/48] intel/fs: Assign constant locations if they haven't been assigned
This sounds good to me, but I guess it is not really fixing anything,
right? I ask because the subject claims that this patch does something
that the original code was already supposed to be doing.
On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> Before, we bailing in assign_constant_locations based on the minimum
> dispatch size. The more direct thing to do is simply to check for
> whether or not we have constant locations and bail if we do. For
> nir_setup_uniforms, it's completely safe to do it multiple times
> because
> we just copy a value from the NIR shader.
> ---
> src/intel/compiler/brw_fs.cpp | 4 +++-
> src/intel/compiler/brw_fs_nir.cpp | 5 -
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index 52079d3..75139fd 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -1956,8 +1956,10 @@ void
> fs_visitor::assign_constant_locations()
> {
> /* Only the first compile gets to decide on locations. */
> - if (dispatch_width != min_dispatch_width)
> + if (push_constant_loc) {
> + assert(pull_constant_loc);
> return;
> + }
>
> bool is_live[uniforms];
> memset(is_live, 0, sizeof(is_live));
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 7556576..05efee3 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs()
> void
> fs_visitor::nir_setup_uniforms()
> {
> - if (dispatch_width != min_dispatch_width)
> + /* Only the first compile gets to set up uniforms. */
> + if (push_constant_loc) {
> + assert(pull_constant_loc);
> return;
> + }
>
> uniforms = nir->num_uniforms / 4;
> }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 23/48] intel/fs: Assign constant locations if they haven't been assigned
On Fri, Oct 27, 2017 at 12:35 AM, Iago Toral wrote:
> This sounds good to me, but I guess it is not really fixing anything,
> right? I ask because the subject claims that this patch does something
> that the original code was already supposed to be doing.
>
This patch is a bit of an artifact of history. I needed it at one point in
the development of the series but I think it may have ended up not
mattering in the end. I still think it's a nice clean-up.
--Jason
> On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> > Before, we bailing in assign_constant_locations based on the minimum
> > dispatch size. The more direct thing to do is simply to check for
> > whether or not we have constant locations and bail if we do. For
> > nir_setup_uniforms, it's completely safe to do it multiple times
> > because
> > we just copy a value from the NIR shader.
> > ---
> > src/intel/compiler/brw_fs.cpp | 4 +++-
> > src/intel/compiler/brw_fs_nir.cpp | 5 -
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_fs.cpp
> > b/src/intel/compiler/brw_fs.cpp
> > index 52079d3..75139fd 100644
> > --- a/src/intel/compiler/brw_fs.cpp
> > +++ b/src/intel/compiler/brw_fs.cpp
> > @@ -1956,8 +1956,10 @@ void
> > fs_visitor::assign_constant_locations()
> > {
> > /* Only the first compile gets to decide on locations. */
> > - if (dispatch_width != min_dispatch_width)
> > + if (push_constant_loc) {
> > + assert(pull_constant_loc);
> >return;
> > + }
> >
> > bool is_live[uniforms];
> > memset(is_live, 0, sizeof(is_live));
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > b/src/intel/compiler/brw_fs_nir.cpp
> > index 7556576..05efee3 100644
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs()
> > void
> > fs_visitor::nir_setup_uniforms()
> > {
> > - if (dispatch_width != min_dispatch_width)
> > + /* Only the first compile gets to set up uniforms. */
> > + if (push_constant_loc) {
> > + assert(pull_constant_loc);
> >return;
> > + }
> >
> > uniforms = nir->num_uniforms / 4;
> > }
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 23/48] intel/fs: Assign constant locations if they haven't been assigned
On Fri, 2017-10-27 at 12:43 -0700, Jason Ekstrand wrote:
> On Fri, Oct 27, 2017 at 12:35 AM, Iago Toral
> wrote:
> > This sounds good to me, but I guess it is not really fixing
> > anything,
> >
> > right? I ask because the subject claims that this patch does
> > something
> >
> > that the original code was already supposed to be doing.
> >
>
> This patch is a bit of an artifact of history. I needed it at one
> point in the development of the series but I think it may have ended
> up not mattering in the end. I still think it's a nice clean-up.
Ok, fair enough. In that case I'd suggest to change the subject to
something like this:
"intel/fs: use pull constant locations to check for first compile of a
shader"
Also, I just noticed something wrong:
(...)
> > > diff --git a/src/intel/compiler/brw_fs.cpp
> >
> > > b/src/intel/compiler/brw_fs.cpp
> >
> > > index 52079d3..75139fd 100644
> >
> > > --- a/src/intel/compiler/brw_fs.cpp
> >
> > > +++ b/src/intel/compiler/brw_fs.cpp
> >
> > > @@ -1956,8 +1956,10 @@ void
> >
> > > fs_visitor::assign_constant_locations()
> >
> > > {
> >
> > > /* Only the first compile gets to decide on locations. */
> >
> > > - if (dispatch_width != min_dispatch_width)
> >
> > > + if (push_constant_loc) {
> >
> > > + assert(pull_constant_loc);
It is impossible that the assert is ever hit. I'd suggest to drop it or
maybe change it by:
assert(dispatch_width != min_dispatch_width);
But since you get rid of min_dispatch_with in the next patch, maybe
just drop the assert.
The same in nir_setup_uniforms below.
> > > return;
> >
> > > + }
> >
> > >
> >
> > > bool is_live[uniforms];
> >
> > > memset(is_live, 0, sizeof(is_live));
> >
> > > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> >
> > > b/src/intel/compiler/brw_fs_nir.cpp
> >
> > > index 7556576..05efee3 100644
> >
> > > --- a/src/intel/compiler/brw_fs_nir.cpp
> >
> > > +++ b/src/intel/compiler/brw_fs_nir.cpp
> >
> > > @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs()
> >
> > > void
> >
> > > fs_visitor::nir_setup_uniforms()
> >
> > > {
> >
> > > - if (dispatch_width != min_dispatch_width)
> >
> > > + /* Only the first compile gets to set up uniforms. */
> >
> > > + if (push_constant_loc) {
> >
> > > + assert(pull_constant_loc);
> > > return;
> >
> > > + }
> >
> > >
> >
> > > uniforms = nir->num_uniforms / 4;
> >
> > > }
> >
> > ___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 23/48] intel/fs: Assign constant locations if they haven't been assigned
On Mon, Oct 30, 2017 at 12:43 AM, Iago Toral wrote:
> On Fri, 2017-10-27 at 12:43 -0700, Jason Ekstrand wrote:
>
> On Fri, Oct 27, 2017 at 12:35 AM, Iago Toral wrote:
>
> This sounds good to me, but I guess it is not really fixing anything,
> right? I ask because the subject claims that this patch does something
> that the original code was already supposed to be doing.
>
>
> This patch is a bit of an artifact of history. I needed it at one point
> in the development of the series but I think it may have ended up not
> mattering in the end. I still think it's a nice clean-up.
>
>
> Ok, fair enough. In that case I'd suggest to change the subject to
> something like this:
>
> "intel/fs: use pull constant locations to check for first compile of a
> shader"
>
Done.
> Also, I just noticed something wrong:
>
> (...)
>
>
> > diff --git a/src/intel/compiler/brw_fs.cpp
> > b/src/intel/compiler/brw_fs.cpp
> > index 52079d3..75139fd 100644
> > --- a/src/intel/compiler/brw_fs.cpp
> > +++ b/src/intel/compiler/brw_fs.cpp
> > @@ -1956,8 +1956,10 @@ void
> > fs_visitor::assign_constant_locations()
> > {
> > /* Only the first compile gets to decide on locations. */
> > - if (dispatch_width != min_dispatch_width)
> > + if (push_constant_loc) {
> > + assert(pull_constant_loc);
>
>
> It is impossible that the assert is ever hit. I'd suggest to drop it or
> maybe change it by:
>
The if condition is for push and the assert is for pull.
--Jason
> assert(dispatch_width != min_dispatch_width);
>
> But since you get rid of min_dispatch_with in the next patch, maybe just
> drop the assert.
>
> The same in nir_setup_uniforms below.
>
> >return;
> > + }
> >
> > bool is_live[uniforms];
> > memset(is_live, 0, sizeof(is_live));
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > b/src/intel/compiler/brw_fs_nir.cpp
> > index 7556576..05efee3 100644
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs()
> > void
> > fs_visitor::nir_setup_uniforms()
> > {
> > - if (dispatch_width != min_dispatch_width)
> > + /* Only the first compile gets to set up uniforms. */
> > + if (push_constant_loc) {
> > + assert(pull_constant_loc);
>
> >return;
> > + }
> >
> > uniforms = nir->num_uniforms / 4;
> > }
>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 23/48] intel/fs: Assign constant locations if they haven't been assigned
On Mon, 2017-10-30 at 11:39 -0700, Jason Ekstrand wrote:
> On Mon, Oct 30, 2017 at 12:43 AM, Iago Toral
> wrote:
> > On Fri, 2017-10-27 at 12:43 -0700, Jason Ekstrand wrote:
> > > On Fri, Oct 27, 2017 at 12:35 AM, Iago Toral
> > > wrote:
> > > > This sounds good to me, but I guess it is not really fixing
> > > > anything,
> > > >
> > > > right? I ask because the subject claims that this patch does
> > > > something
> > > >
> > > > that the original code was already supposed to be doing.
> > > >
> > >
> > > This patch is a bit of an artifact of history. I needed it at
> > > one point in the development of the series but I think it may
> > > have ended up not mattering in the end. I still think it's a
> > > nice clean-up.
> >
> > Ok, fair enough. In that case I'd suggest to change the subject to
> > something like this:
> >
> > "intel/fs: use pull constant locations to check for first compile
> > of a shader"
>
> Done.
>
> > Also, I just noticed something wrong:
> >
> > (...)
> >
> > > > > diff --git a/src/intel/compiler/brw_fs.cpp
> > > >
> > > > > b/src/intel/compiler/brw_fs.cpp
> > > >
> > > > > index 52079d3..75139fd 100644
> > > >
> > > > > --- a/src/intel/compiler/brw_fs.cpp
> > > >
> > > > > +++ b/src/intel/compiler/brw_fs.cpp
> > > >
> > > > > @@ -1956,8 +1956,10 @@ void
> > > >
> > > > > fs_visitor::assign_constant_locations()
> > > >
> > > > > {
> > > >
> > > > > /* Only the first compile gets to decide on locations. */
> > > >
> > > > > - if (dispatch_width != min_dispatch_width)
> > > >
> > > > > + if (push_constant_loc) {
> > > >
> > > > > + assert(pull_constant_loc);
> >
> > It is impossible that the assert is ever hit. I'd suggest to drop
> > it or maybe change it by:
>
> The if condition is for push and the assert is for pull.
Wow! I read it twice because I thought you would be doing that and I
read it wrong both times, sorry about that :-(
> --Jason
>
> > assert(dispatch_width != min_dispatch_width);
> >
> > But since you get rid of min_dispatch_with in the next patch, maybe
> > just drop the assert.
> >
> > The same in nir_setup_uniforms below.
> >
> > > > > return;
> > > >
> > > > > + }
> > > >
> > > > >
> > > >
> > > > > bool is_live[uniforms];
> > > >
> > > > > memset(is_live, 0, sizeof(is_live));
> > > >
> > > > > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > > >
> > > > > b/src/intel/compiler/brw_fs_nir.cpp
> > > >
> > > > > index 7556576..05efee3 100644
> > > >
> > > > > --- a/src/intel/compiler/brw_fs_nir.cpp
> > > >
> > > > > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > > >
> > > > > @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs()
> > > >
> > > > > void
> > > >
> > > > > fs_visitor::nir_setup_uniforms()
> > > >
> > > > > {
> > > >
> > > > > - if (dispatch_width != min_dispatch_width)
> > > >
> > > > > + /* Only the first compile gets to set up uniforms. */
> > > >
> > > > > + if (push_constant_loc) {
> > > >
> > > > > + assert(pull_constant_loc);
> > > > > return;
> > > >
> > > > > + }
> > > >
> > > > >
> > > >
> > > > > uniforms = nir->num_uniforms / 4;
> > > >
> > > > > }
> > > >
> > > > ___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
