Re: [Mesa-dev] [PATCH 02/13] nir: add load_param

[Karol Herbst]

On Fri, Mar 2, 2018 at 7:02 AM, Jason Ekstrand  wrote:
> On Wed, Feb 28, 2018 at 1:25 PM, Rob Clark  wrote:
>>
>> On Wed, Feb 28, 2018 at 4:16 PM, Eric Anholt  wrote:
>> > Rob Clark  writes:
>> >
>> >> From: Karol Herbst 
>> >>
>> >> OpenCL kernels have parameters (see pipe_grid_info::input), and so we
>> >> need a way to access them.
>> >>
>> >> Signed-off-by: Rob Clark 
>> >>
>> >> ---
>> >>  src/compiler/nir/nir_intrinsics.h |  2 ++
>> >>  src/compiler/nir/nir_lower_io.c   | 13 ++---
>> >>  2 files changed, 12 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/src/compiler/nir/nir_intrinsics.h
>> >> b/src/compiler/nir/nir_intrinsics.h
>> >> index ede29277876..0915c5e809f 100644
>> >> --- a/src/compiler/nir/nir_intrinsics.h
>> >> +++ b/src/compiler/nir/nir_intrinsics.h
>> >> @@ -435,6 +435,8 @@ LOAD(ubo, 2, 0, xx, xx, xx,
>> >> NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REOR
>> >>  LOAD(input, 1, 2, BASE, COMPONENT, xx, NIR_INTRINSIC_CAN_ELIMINATE |
>> >> NIR_INTRINSIC_CAN_REORDER)
>> >>  /* src[] = { vertex, offset }. const_index[] = { base, component } */
>> >>  LOAD(per_vertex_input, 2, 2, BASE, COMPONENT, xx,
>> >> NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
>> >> +/* src[] = { }. const_index[] = { base } */
>> >> +LOAD(param, 0, 1, BASE, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE |
>> >> NIR_INTRINSIC_CAN_REORDER)
>> >
>> > I know this is a new request compared to the existing pattern, but could
>> > you put a comment describing what load_param does in the code as well as
>> > the commit message?  Especially what the meaning of the base is.  We've
>> > been bad at this in NIR, and it makes learning how to write a new NIR
>> > backend more challenging than it should be.
>> >
>> > Also, what makes these params different from UBOs or default uniforms?
>>
>> yeah, I guess makes sense to describe better in code.. but for now
>> base is just the parameter number (ie. first param base=0, second
>> param base=1, etc)
>>
>> For ir3, I end up uploading these as uniforms.. I'm not sure how nv
>> does kernel params offhand.  Maybe if they just end up uniforms for
>> everyone we can change clover to use ctx->set_constant_buffer() and
>> use a pass to lower load_param to load_uniform.  At least that would
>> solve one awkward thing about the pipe driver and clover agreeing on
>> the layout of pipe_grid_info::input.
>
>
> Disclaimer: I won't claim to be an OpenCL expert.  I had a chat with Curro
> this morning and I think I have a decent handle on how it works now but I
> may be missing something.
>
> That said, I suspect that load/store_param is the wrong approach.  My
> understanding is that each type of parameter: pointer, image, constant, etc.
> has a corresponding binding type in GL that's a fairly close match: SSBO,
> image, UBO, etc.  If this is true, then I think the better thing to do would
> be to translate parameters in the main function into vtn_variables of the
> appropriate type.  OpenCL can use a very simple binding model where you
> index by the order they show up in the parameter list.  For globals, I'm not
> sure how OpenCL gives you access to them but I would guess some fairly
> straightforward binding concept can be used there as well.
>
> --Jason

there won't be a store_param. And load_param should just read out the
value from the const buffer (or whatever the driver uses) to pass
those values into the kernel. Like you use load_param to load the
pointer value or image reference and so on, and use the specialized
load/store operations to deal with those types.

Basically you get a big buffer containing all the values you passed in
via clSetKernelArg (except sampler I think, those are just handled
internally) and you use load_param to just read from this input
buffer. Then you get for example a pointer into global memory and can
use this pointer to do global memory operations.

I think we shouldn't map global pointer to SSBOs though, because they
are full 64 bit pointers into anywhere in global memory. That's why we
added the load/store_global intrinsics to deal with those. Atomics
will be interesting here because you also have atomic functions on
local memory, not just global memory like in glsl. But this is
independent from kernel parameter loading anyway.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/13] nir: add load_param

[Jason Ekstrand]

On Wed, Feb 28, 2018 at 1:25 PM, Rob Clark  wrote:

> On Wed, Feb 28, 2018 at 4:16 PM, Eric Anholt  wrote:
> > Rob Clark  writes:
> >
> >> From: Karol Herbst 
> >>
> >> OpenCL kernels have parameters (see pipe_grid_info::input), and so we
> >> need a way to access them.
> >>
> >> Signed-off-by: Rob Clark 
> >>
> >> ---
> >>  src/compiler/nir/nir_intrinsics.h |  2 ++
> >>  src/compiler/nir/nir_lower_io.c   | 13 ++---
> >>  2 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/compiler/nir/nir_intrinsics.h b/src/compiler/nir/nir_
> intrinsics.h
> >> index ede29277876..0915c5e809f 100644
> >> --- a/src/compiler/nir/nir_intrinsics.h
> >> +++ b/src/compiler/nir/nir_intrinsics.h
> >> @@ -435,6 +435,8 @@ LOAD(ubo, 2, 0, xx, xx, xx,
> NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REOR
> >>  LOAD(input, 1, 2, BASE, COMPONENT, xx, NIR_INTRINSIC_CAN_ELIMINATE |
> NIR_INTRINSIC_CAN_REORDER)
> >>  /* src[] = { vertex, offset }. const_index[] = { base, component } */
> >>  LOAD(per_vertex_input, 2, 2, BASE, COMPONENT, xx,
> NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
> >> +/* src[] = { }. const_index[] = { base } */
> >> +LOAD(param, 0, 1, BASE, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE |
> NIR_INTRINSIC_CAN_REORDER)
> >
> > I know this is a new request compared to the existing pattern, but could
> > you put a comment describing what load_param does in the code as well as
> > the commit message?  Especially what the meaning of the base is.  We've
> > been bad at this in NIR, and it makes learning how to write a new NIR
> > backend more challenging than it should be.
> >
> > Also, what makes these params different from UBOs or default uniforms?
>
> yeah, I guess makes sense to describe better in code.. but for now
> base is just the parameter number (ie. first param base=0, second
> param base=1, etc)
>
> For ir3, I end up uploading these as uniforms.. I'm not sure how nv
> does kernel params offhand.  Maybe if they just end up uniforms for
> everyone we can change clover to use ctx->set_constant_buffer() and
> use a pass to lower load_param to load_uniform.  At least that would
> solve one awkward thing about the pipe driver and clover agreeing on
> the layout of pipe_grid_info::input.
>

Disclaimer: I won't claim to be an OpenCL expert.  I had a chat with Curro
this morning and I think I have a decent handle on how it works now but I
may be missing something.

That said, I suspect that load/store_param is the wrong approach.  My
understanding is that each type of parameter: pointer, image, constant,
etc. has a corresponding binding type in GL that's a fairly close match:
SSBO, image, UBO, etc.  If this is true, then I think the better thing to
do would be to translate parameters in the main function into vtn_variables
of the appropriate type.  OpenCL can use a very simple binding model where
you index by the order they show up in the parameter list.  For globals,
I'm not sure how OpenCL gives you access to them but I would guess some
fairly straightforward binding concept can be used there as well.

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/13] nir: add load_param

[Rob Clark]

On Wed, Feb 28, 2018 at 4:16 PM, Eric Anholt  wrote:
> Rob Clark  writes:
>
>> From: Karol Herbst 
>>
>> OpenCL kernels have parameters (see pipe_grid_info::input), and so we
>> need a way to access them.
>>
>> Signed-off-by: Rob Clark 
>>
>> ---
>>  src/compiler/nir/nir_intrinsics.h |  2 ++
>>  src/compiler/nir/nir_lower_io.c   | 13 ++---
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir_intrinsics.h 
>> b/src/compiler/nir/nir_intrinsics.h
>> index ede29277876..0915c5e809f 100644
>> --- a/src/compiler/nir/nir_intrinsics.h
>> +++ b/src/compiler/nir/nir_intrinsics.h
>> @@ -435,6 +435,8 @@ LOAD(ubo, 2, 0, xx, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE 
>> | NIR_INTRINSIC_CAN_REOR
>>  LOAD(input, 1, 2, BASE, COMPONENT, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
>> NIR_INTRINSIC_CAN_REORDER)
>>  /* src[] = { vertex, offset }. const_index[] = { base, component } */
>>  LOAD(per_vertex_input, 2, 2, BASE, COMPONENT, xx, 
>> NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
>> +/* src[] = { }. const_index[] = { base } */
>> +LOAD(param, 0, 1, BASE, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
>> NIR_INTRINSIC_CAN_REORDER)
>
> I know this is a new request compared to the existing pattern, but could
> you put a comment describing what load_param does in the code as well as
> the commit message?  Especially what the meaning of the base is.  We've
> been bad at this in NIR, and it makes learning how to write a new NIR
> backend more challenging than it should be.
>
> Also, what makes these params different from UBOs or default uniforms?

yeah, I guess makes sense to describe better in code.. but for now
base is just the parameter number (ie. first param base=0, second
param base=1, etc)

For ir3, I end up uploading these as uniforms.. I'm not sure how nv
does kernel params offhand.  Maybe if they just end up uniforms for
everyone we can change clover to use ctx->set_constant_buffer() and
use a pass to lower load_param to load_uniform.  At least that would
solve one awkward thing about the pipe driver and clover agreeing on
the layout of pipe_grid_info::input.

BR,
-R
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/13] nir: add load_param

[Eric Anholt]

Rob Clark  writes:

> From: Karol Herbst 
>
> OpenCL kernels have parameters (see pipe_grid_info::input), and so we
> need a way to access them.
>
> Signed-off-by: Rob Clark 
>
> ---
>  src/compiler/nir/nir_intrinsics.h |  2 ++
>  src/compiler/nir/nir_lower_io.c   | 13 ++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/nir/nir_intrinsics.h 
> b/src/compiler/nir/nir_intrinsics.h
> index ede29277876..0915c5e809f 100644
> --- a/src/compiler/nir/nir_intrinsics.h
> +++ b/src/compiler/nir/nir_intrinsics.h
> @@ -435,6 +435,8 @@ LOAD(ubo, 2, 0, xx, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
> NIR_INTRINSIC_CAN_REOR
>  LOAD(input, 1, 2, BASE, COMPONENT, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
> NIR_INTRINSIC_CAN_REORDER)
>  /* src[] = { vertex, offset }. const_index[] = { base, component } */
>  LOAD(per_vertex_input, 2, 2, BASE, COMPONENT, xx, 
> NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
> +/* src[] = { }. const_index[] = { base } */
> +LOAD(param, 0, 1, BASE, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
> NIR_INTRINSIC_CAN_REORDER)

I know this is a new request compared to the existing pattern, but could
you put a comment describing what load_param does in the code as well as
the commit message?  Especially what the meaning of the base is.  We've
been bad at this in NIR, and it makes learning how to write a new NIR
backend more challenging than it should be.

Also, what makes these params different from UBOs or default uniforms?


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 02/13] nir: add load_param

[Rob Clark]

From: Karol Herbst 

OpenCL kernels have parameters (see pipe_grid_info::input), and so we
need a way to access them.

Signed-off-by: Rob Clark 
---
 src/compiler/nir/nir_intrinsics.h |  2 ++
 src/compiler/nir/nir_lower_io.c   | 13 ++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/compiler/nir/nir_intrinsics.h 
b/src/compiler/nir/nir_intrinsics.h
index ede29277876..0915c5e809f 100644
--- a/src/compiler/nir/nir_intrinsics.h
+++ b/src/compiler/nir/nir_intrinsics.h
@@ -435,6 +435,8 @@ LOAD(ubo, 2, 0, xx, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
NIR_INTRINSIC_CAN_REOR
 LOAD(input, 1, 2, BASE, COMPONENT, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
NIR_INTRINSIC_CAN_REORDER)
 /* src[] = { vertex, offset }. const_index[] = { base, component } */
 LOAD(per_vertex_input, 2, 2, BASE, COMPONENT, xx, NIR_INTRINSIC_CAN_ELIMINATE 
| NIR_INTRINSIC_CAN_REORDER)
+/* src[] = { }. const_index[] = { base } */
+LOAD(param, 0, 1, BASE, xx, xx, NIR_INTRINSIC_CAN_ELIMINATE | 
NIR_INTRINSIC_CAN_REORDER)
 /* src[] = { barycoord, offset }. const_index[] = { base, component } */
 INTRINSIC(load_interpolated_input, 2, ARR(2, 1), true, 0, 0,
   2, BASE, COMPONENT, xx,
diff --git a/src/compiler/nir/nir_lower_io.c b/src/compiler/nir/nir_lower_io.c
index df91febd68d..febd28b560e 100644
--- a/src/compiler/nir/nir_lower_io.c
+++ b/src/compiler/nir/nir_lower_io.c
@@ -199,6 +199,9 @@ lower_load(nir_intrinsic_instr *intrin, struct 
lower_io_state *state,
case nir_var_shared:
   op = nir_intrinsic_load_shared;
   break;
+   case nir_var_param:
+  op = nir_intrinsic_load_param;
+  break;
default:
   unreachable("Unknown variable mode");
}
@@ -207,7 +210,10 @@ lower_load(nir_intrinsic_instr *intrin, struct 
lower_io_state *state,
   nir_intrinsic_instr_create(state->builder.shader, op);
load->num_components = intrin->num_components;
 
-   nir_intrinsic_set_base(load, var->data.driver_location);
+   if (mode == nir_var_param)
+  nir_intrinsic_set_base(load, var->data.location);
+   else
+  nir_intrinsic_set_base(load, var->data.driver_location);
if (mode == nir_var_shader_in || mode == nir_var_shader_out)
   nir_intrinsic_set_component(load, component);
 
@@ -220,7 +226,7 @@ lower_load(nir_intrinsic_instr *intrin, struct 
lower_io_state *state,
} else if (barycentric) {
   load->src[0] = nir_src_for_ssa(barycentric);
   load->src[1] = nir_src_for_ssa(offset);
-   } else {
+   } else if (mode != nir_var_param) {
   load->src[0] = nir_src_for_ssa(offset);
}
 
@@ -407,7 +413,8 @@ nir_lower_io_block(nir_block *block,
   if (mode != nir_var_shader_in &&
   mode != nir_var_shader_out &&
   mode != nir_var_shared &&
-  mode != nir_var_uniform)
+  mode != nir_var_uniform &&
+  mode != nir_var_param)
  continue;
 
   b->cursor = nir_before_instr(instr);
-- 
2.14.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev