[RFC,1/4] nir: Add a new intrinsic 'load_image_stride'

Submitted by Eduardo Lima Mitev on Jan. 25, 2019, 3:48 p.m.

Details

Message ID 20190125154833.25757-2-elima@igalia.com
State New
Headers show
Series "freedreno: Move some compiler offset computations to NIR" ( rev: 1 ) in Freedreno

Not browsing as part of any series.

Commit Message

Eduardo Lima Mitev Jan. 25, 2019, 3:48 p.m.
This is an internal intrinsic intended to be injected by a
(freedreno-specific) 'lower_sampler_io' pass that will be introduced
later in this series; and consumed by ir3_compiler_nir.

The intrinsic will load in SSA values for various constants
for images (image_dims), namely the format's bytes-per-pixel,
y-stride and z-stride; for which the backend compiler will emit
the corresponding uniforms.

const_index[0] is the image index, and const_index[1] is the index
into image_dims' register file for a given image: 0 for bpp, 1 to
y-stride and 2 for z-stride.
---
 src/compiler/nir/nir_intrinsics.py | 2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/compiler/nir/nir_intrinsics.py b/src/compiler/nir/nir_intrinsics.py
index a5cc3f7401c..169c835d746 100644
--- a/src/compiler/nir/nir_intrinsics.py
+++ b/src/compiler/nir/nir_intrinsics.py
@@ -590,6 +590,8 @@  load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET], [CAN_ELIMINATE])
 load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
 # src[] = { offset }. const_index[] = { base, range }
 load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
+# src[] = { offset }. const_index[] = { image_idx, dim_idx }
+load("image_stride", 1, [], [CAN_REORDER])
 
 # Stores work the same way as loads, except now the first source is the value
 # to store and the second (and possibly third) source specify where to store

Comments

Eduardo Lima Mitev <elima@igalia.com> writes:

> This is an internal intrinsic intended to be injected by a
> (freedreno-specific) 'lower_sampler_io' pass that will be introduced
> later in this series; and consumed by ir3_compiler_nir.
>
> The intrinsic will load in SSA values for various constants
> for images (image_dims), namely the format's bytes-per-pixel,
> y-stride and z-stride; for which the backend compiler will emit
> the corresponding uniforms.
>
> const_index[0] is the image index, and const_index[1] is the index
> into image_dims' register file for a given image: 0 for bpp, 1 to
> y-stride and 2 for z-stride.

Can you move this documentation of the meaning of the intrinsic into a
comment in the file?
On 1/25/19 6:42 PM, Eric Anholt wrote:
> Eduardo Lima Mitev <elima@igalia.com> writes:
> 
>> This is an internal intrinsic intended to be injected by a
>> (freedreno-specific) 'lower_sampler_io' pass that will be introduced
>> later in this series; and consumed by ir3_compiler_nir.
>>
>> The intrinsic will load in SSA values for various constants
>> for images (image_dims), namely the format's bytes-per-pixel,
>> y-stride and z-stride; for which the backend compiler will emit
>> the corresponding uniforms.
>>
>> const_index[0] is the image index, and const_index[1] is the index
>> into image_dims' register file for a given image: 0 for bpp, 1 to
>> y-stride and 2 for z-stride.
> 
> Can you move this documentation of the meaning of the intrinsic into a
> comment in the file?
> 

Yes, will do that.
Thanks!
Mind suffixing it with _ir3 or something since it's a back-end-specific 
intrinsic?  Incidentally, this looks a lot like load_image_param_intel.

On January 25, 2019 07:48:54 Eduardo Lima Mitev <elima@igalia.com> wrote:

> This is an internal intrinsic intended to be injected by a
> (freedreno-specific) 'lower_sampler_io' pass that will be introduced
> later in this series; and consumed by ir3_compiler_nir.
>
> The intrinsic will load in SSA values for various constants
> for images (image_dims), namely the format's bytes-per-pixel,
> y-stride and z-stride; for which the backend compiler will emit
> the corresponding uniforms.
>
> const_index[0] is the image index, and const_index[1] is the index
> into image_dims' register file for a given image: 0 for bpp, 1 to
> y-stride and 2 for z-stride.
> ---
> src/compiler/nir/nir_intrinsics.py | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/compiler/nir/nir_intrinsics.py 
> b/src/compiler/nir/nir_intrinsics.py
> index a5cc3f7401c..169c835d746 100644
> --- a/src/compiler/nir/nir_intrinsics.py
> +++ b/src/compiler/nir/nir_intrinsics.py
> @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET], 
> [CAN_ELIMINATE])
> load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
> # src[] = { offset }. const_index[] = { base, range }
> load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
> +# src[] = { offset }. const_index[] = { image_idx, dim_idx }
> +load("image_stride", 1, [], [CAN_REORDER])
>
> # Stores work the same way as loads, except now the first source is the value
> # to store and the second (and possibly third) source specify where to store
> --
> 2.20.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 1/26/19 5:34 PM, Jason Ekstrand wrote:
> Mind suffixing it with _ir3 or something since it's a back-end-specific
> intrinsic?  Incidentally, this looks a lot like load_image_param_intel.
> 

Yes, I felt inclined to add the suffix but wasn't sure how good/bad a
practice it is, so I deferred it to review :).

Now, I did a quick experiment and it turns out I can reuse
image_deref_load_param_intel as-is. The semantics are a bit different so
I would have to fork the comment to explain ir3 too.

So, what about I remove the '_intel' suffix to that one and use if for
this ir3?

Thanks for the feedback and the tip!

Eduardo

> On January 25, 2019 07:48:54 Eduardo Lima Mitev <elima@igalia.com> wrote:
> 
>> This is an internal intrinsic intended to be injected by a
>> (freedreno-specific) 'lower_sampler_io' pass that will be introduced
>> later in this series; and consumed by ir3_compiler_nir.
>>
>> The intrinsic will load in SSA values for various constants
>> for images (image_dims), namely the format's bytes-per-pixel,
>> y-stride and z-stride; for which the backend compiler will emit
>> the corresponding uniforms.
>>
>> const_index[0] is the image index, and const_index[1] is the index
>> into image_dims' register file for a given image: 0 for bpp, 1 to
>> y-stride and 2 for z-stride.
>> ---
>> src/compiler/nir/nir_intrinsics.py | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/src/compiler/nir/nir_intrinsics.py
>> b/src/compiler/nir/nir_intrinsics.py
>> index a5cc3f7401c..169c835d746 100644
>> --- a/src/compiler/nir/nir_intrinsics.py
>> +++ b/src/compiler/nir/nir_intrinsics.py
>> @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET],
>> [CAN_ELIMINATE])
>> load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
>> # src[] = { offset }. const_index[] = { base, range }
>> load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
>> +# src[] = { offset }. const_index[] = { image_idx, dim_idx }
>> +load("image_stride", 1, [], [CAN_REORDER])
>>
>> # Stores work the same way as loads, except now the first source is
>> the value
>> # to store and the second (and possibly third) source specify where to
>> store
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
> 
>
On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev <elima@igalia.com> wrote:
>
> On 1/26/19 5:34 PM, Jason Ekstrand wrote:
> > Mind suffixing it with _ir3 or something since it's a back-end-specific
> > intrinsic?  Incidentally, this looks a lot like load_image_param_intel.
> >
>
> Yes, I felt inclined to add the suffix but wasn't sure how good/bad a
> practice it is, so I deferred it to review :).
>
> Now, I did a quick experiment and it turns out I can reuse
> image_deref_load_param_intel as-is. The semantics are a bit different so
> I would have to fork the comment to explain ir3 too.
>
> So, what about I remove the '_intel' suffix to that one and use if for
> this ir3?

Instructions that have different semantics for different drivers sound
like a lot of potential for confusion and latent bugs to me. Is it
really a problem to have both?
>
> Thanks for the feedback and the tip!
>
> Eduardo
>
> > On January 25, 2019 07:48:54 Eduardo Lima Mitev <elima@igalia.com> wrote:
> >
> >> This is an internal intrinsic intended to be injected by a
> >> (freedreno-specific) 'lower_sampler_io' pass that will be introduced
> >> later in this series; and consumed by ir3_compiler_nir.
> >>
> >> The intrinsic will load in SSA values for various constants
> >> for images (image_dims), namely the format's bytes-per-pixel,
> >> y-stride and z-stride; for which the backend compiler will emit
> >> the corresponding uniforms.
> >>
> >> const_index[0] is the image index, and const_index[1] is the index
> >> into image_dims' register file for a given image: 0 for bpp, 1 to
> >> y-stride and 2 for z-stride.
> >> ---
> >> src/compiler/nir/nir_intrinsics.py | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/src/compiler/nir/nir_intrinsics.py
> >> b/src/compiler/nir/nir_intrinsics.py
> >> index a5cc3f7401c..169c835d746 100644
> >> --- a/src/compiler/nir/nir_intrinsics.py
> >> +++ b/src/compiler/nir/nir_intrinsics.py
> >> @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET],
> >> [CAN_ELIMINATE])
> >> load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
> >> # src[] = { offset }. const_index[] = { base, range }
> >> load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
> >> +# src[] = { offset }. const_index[] = { image_idx, dim_idx }
> >> +load("image_stride", 1, [], [CAN_REORDER])
> >>
> >> # Stores work the same way as loads, except now the first source is
> >> the value
> >> # to store and the second (and possibly third) source specify where to
> >> store
> >> --
> >> 2.20.1
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> >
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 1/28/19 9:56 AM, Bas Nieuwenhuizen wrote:
> On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev <elima@igalia.com> wrote:
>>
>> On 1/26/19 5:34 PM, Jason Ekstrand wrote:
>>> Mind suffixing it with _ir3 or something since it's a back-end-specific
>>> intrinsic?  Incidentally, this looks a lot like load_image_param_intel.
>>>
>>
>> Yes, I felt inclined to add the suffix but wasn't sure how good/bad a
>> practice it is, so I deferred it to review :).
>>
>> Now, I did a quick experiment and it turns out I can reuse
>> image_deref_load_param_intel as-is. The semantics are a bit different so
>> I would have to fork the comment to explain ir3 too.
>>
>> So, what about I remove the '_intel' suffix to that one and use if for
>> this ir3?
> 
> Instructions that have different semantics for different drivers sound
> like a lot of potential for confusion and latent bugs to me. Is it
> really a problem to have both?
>

It is not a problem at all, but I think it is preferable not to
duplicate this intrinsic if it serves the same purpose on both backends,
and only slightly differ in the specific set of params it holds:

On both backends the intrinsic is used to represent registers at compile
time, that will be resolved to uniforms at shader run time.

On intel, the params are offset, tiling, stride and swizzling; on ir3
they are bytes-per-pixel, y-stride and z-stride. That's what I mean by
different semantics, and I think the difference is not enough to justify
forking it.

I'm ok either way, though.

Eduardo

>>
>> Thanks for the feedback and the tip!
>>
>> Eduardo
>>
>>> On January 25, 2019 07:48:54 Eduardo Lima Mitev <elima@igalia.com> wrote:
>>>
>>>> This is an internal intrinsic intended to be injected by a
>>>> (freedreno-specific) 'lower_sampler_io' pass that will be introduced
>>>> later in this series; and consumed by ir3_compiler_nir.
>>>>
>>>> The intrinsic will load in SSA values for various constants
>>>> for images (image_dims), namely the format's bytes-per-pixel,
>>>> y-stride and z-stride; for which the backend compiler will emit
>>>> the corresponding uniforms.
>>>>
>>>> const_index[0] is the image index, and const_index[1] is the index
>>>> into image_dims' register file for a given image: 0 for bpp, 1 to
>>>> y-stride and 2 for z-stride.
>>>> ---
>>>> src/compiler/nir/nir_intrinsics.py | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/compiler/nir/nir_intrinsics.py
>>>> b/src/compiler/nir/nir_intrinsics.py
>>>> index a5cc3f7401c..169c835d746 100644
>>>> --- a/src/compiler/nir/nir_intrinsics.py
>>>> +++ b/src/compiler/nir/nir_intrinsics.py
>>>> @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET],
>>>> [CAN_ELIMINATE])
>>>> load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
>>>> # src[] = { offset }. const_index[] = { base, range }
>>>> load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
>>>> +# src[] = { offset }. const_index[] = { image_idx, dim_idx }
>>>> +load("image_stride", 1, [], [CAN_REORDER])
>>>>
>>>> # Stores work the same way as loads, except now the first source is
>>>> the value
>>>> # to store and the second (and possibly third) source specify where to
>>>> store
>>>> --
>>>> 2.20.1
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>
>>>
>>>
>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On 1/28/19 10:16 AM, Eduardo Lima Mitev wrote:
> On 1/28/19 9:56 AM, Bas Nieuwenhuizen wrote:
>> On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev <elima@igalia.com> wrote:
>>>
>>> On 1/26/19 5:34 PM, Jason Ekstrand wrote:
>>>> Mind suffixing it with _ir3 or something since it's a back-end-specific
>>>> intrinsic?  Incidentally, this looks a lot like load_image_param_intel.
>>>>
>>>
>>> Yes, I felt inclined to add the suffix but wasn't sure how good/bad a
>>> practice it is, so I deferred it to review :).
>>>
>>> Now, I did a quick experiment and it turns out I can reuse
>>> image_deref_load_param_intel as-is. The semantics are a bit different so
>>> I would have to fork the comment to explain ir3 too.
>>>
>>> So, what about I remove the '_intel' suffix to that one and use if for
>>> this ir3?
>>
>> Instructions that have different semantics for different drivers sound
>> like a lot of potential for confusion and latent bugs to me. Is it
>> really a problem to have both?
>>
> 
> It is not a problem at all, but I think it is preferable not to
> duplicate this intrinsic if it serves the same purpose on both backends,
> and only slightly differ in the specific set of params it holds:
> 
> On both backends the intrinsic is used to represent registers at compile
> time, that will be resolved to uniforms at shader run time.
> 
> On intel, the params are offset, tiling, stride and swizzling; on ir3
> they are bytes-per-pixel, y-stride and z-stride. That's what I mean by
> different semantics, and I think the difference is not enough to justify
> forking it.
> 

Actually, one can argue there is no semantic difference at all from
NIR's point of view. The intrinsic just load certain image params on
both backends, and it is only backend-specific what those params are.

> I'm ok either way, though.
> 
> Eduardo
> 
>>>
>>> Thanks for the feedback and the tip!
>>>
>>> Eduardo
>>>
>>>> On January 25, 2019 07:48:54 Eduardo Lima Mitev <elima@igalia.com> wrote:
>>>>
>>>>> This is an internal intrinsic intended to be injected by a
>>>>> (freedreno-specific) 'lower_sampler_io' pass that will be introduced
>>>>> later in this series; and consumed by ir3_compiler_nir.
>>>>>
>>>>> The intrinsic will load in SSA values for various constants
>>>>> for images (image_dims), namely the format's bytes-per-pixel,
>>>>> y-stride and z-stride; for which the backend compiler will emit
>>>>> the corresponding uniforms.
>>>>>
>>>>> const_index[0] is the image index, and const_index[1] is the index
>>>>> into image_dims' register file for a given image: 0 for bpp, 1 to
>>>>> y-stride and 2 for z-stride.
>>>>> ---
>>>>> src/compiler/nir/nir_intrinsics.py | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/src/compiler/nir/nir_intrinsics.py
>>>>> b/src/compiler/nir/nir_intrinsics.py
>>>>> index a5cc3f7401c..169c835d746 100644
>>>>> --- a/src/compiler/nir/nir_intrinsics.py
>>>>> +++ b/src/compiler/nir/nir_intrinsics.py
>>>>> @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET],
>>>>> [CAN_ELIMINATE])
>>>>> load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
>>>>> # src[] = { offset }. const_index[] = { base, range }
>>>>> load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER])
>>>>> +# src[] = { offset }. const_index[] = { image_idx, dim_idx }
>>>>> +load("image_stride", 1, [], [CAN_REORDER])
>>>>>
>>>>> # Stores work the same way as loads, except now the first source is
>>>>> the value
>>>>> # to store and the second (and possibly third) source specify where to
>>>>> store
>>>>> --
>>>>> 2.20.1
>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>
>>>>
>>>>
>>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>