Re: [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
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 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 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
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
On 1/28/19 9:56 AM, Bas Nieuwenhuizen wrote:
> On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev 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 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
Re: [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev 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 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
Re: [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
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 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
Re: [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
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 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
Re: [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
On 1/25/19 6:42 PM, Eric Anholt wrote: > Eduardo Lima Mitev 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! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
Eduardo Lima Mitev 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? signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
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
