Re: [Mesa-dev] [PATCH 19/29] nir: fix up bit sizes for undefined alu sources

[Samuel Iglesias Gonsálvez]

On 22/03/16 17:37, Jason Ekstrand wrote:
> On Mar 22, 2016 8:18 AM, "Samuel Iglesias Gonsálvez" 
> wrote:
>>
>>
>>
>> On 21/03/16 23:54, Jason Ekstrand wrote:
>>> On Mon, Mar 21, 2016 at 5:05 AM, Samuel Iglesias Gonsálvez <
>>> sigles...@igalia.com> wrote:
>>>
 From: Iago Toral Quiroga 

 Undefined sources in alu operations don't have a valid bit size because
 they are uninitialized. Simply ignoring undefined sources for bit size
 validation is not enough since drivers can check and operate with the
 bit-size and that can lead to issues later on. Instead, fix undefined
 sources to always have a compatible bit size.

>>>
>>> I'm not sure what I think about this.  I think I'd rather have undefs
>>> simply have the right bitsize.
>>>
>>
>> With undefined sources you cannot get the bitsize from themselves
>> because it is not initialized.
> 
> Doesn't patch 3 and the discussion on it imply that undefs should have
> valid sizes?
> 

Oh right. I will discard this patch as undefs already have valid sizes.

Thanks,

Sam

>> In that case, we pick the bit size from
>> the ALU opcode's input definition. If it is unsized, then we use the
>> destination size.
> 
> I dont think pulling the implicit size from the source size is ever
> correct.  If you have an explicitly sized source that means it doesn't
> affect and isn't affected by the implicit size.
> 
>> I think this is the right bitsize... or am I missing something?
>>
>> Sam
>>
>>>
 v2 (Sam):
 - Use helper to get type size from nir_alu_type.
 ---
  src/compiler/nir/nir_validate.c | 10 ++
  1 file changed, 10 insertions(+)

 diff --git a/src/compiler/nir/nir_validate.c
 b/src/compiler/nir/nir_validate.c
 index 9f18d1c..645c15a 100644
 --- a/src/compiler/nir/nir_validate.c
 +++ b/src/compiler/nir/nir_validate.c
 @@ -180,9 +180,11 @@ validate_alu_src(nir_alu_instr *instr, unsigned
 index, validate_state *state)

 unsigned num_components;
 unsigned src_bit_size;
 +   bool is_undef = false;
 if (src->src.is_ssa) {
src_bit_size = src->src.ssa->bit_size;
num_components = src->src.ssa->num_components;
 +  is_undef = src->src.ssa->parent_instr->type ==
 nir_instr_type_ssa_undef;
 } else {
src_bit_size = src->src.reg.reg->bit_size;
if (src->src.reg.reg->is_packed)
 @@ -205,12 +207,20 @@ validate_alu_src(nir_alu_instr *instr, unsigned
 index, validate_state *state)

 if (nir_alu_type_get_type_size(src_type)) {
/* This source has an explicit bit size */
 +  if (is_undef) {
 + src_bit_size = nir_alu_type_get_type_size(src_type);
 + src->src.ssa->bit_size = src_bit_size;
 +  }
assert(nir_alu_type_get_type_size(src_type) == src_bit_size);
 } else {
if
 (!nir_alu_type_get_type_size(nir_op_infos[instr->op].output_type)) {
   unsigned dest_bit_size =
  instr->dest.dest.is_ssa ? instr->dest.dest.ssa.bit_size
  :
> instr->dest.dest.reg.reg->bit_size;
 + if (is_undef) {
 +src_bit_size = dest_bit_size;
 +src->src.ssa->bit_size = dest_bit_size;
 + }
   assert(dest_bit_size == src_bit_size);
}
 }
 --
 2.5.0

 ___
 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] [PATCH 19/29] nir: fix up bit sizes for undefined alu sources

[Jason Ekstrand]

On Mar 22, 2016 8:18 AM, "Samuel Iglesias Gonsálvez" 
wrote:
>
>
>
> On 21/03/16 23:54, Jason Ekstrand wrote:
> > On Mon, Mar 21, 2016 at 5:05 AM, Samuel Iglesias Gonsálvez <
> > sigles...@igalia.com> wrote:
> >
> >> From: Iago Toral Quiroga 
> >>
> >> Undefined sources in alu operations don't have a valid bit size because
> >> they are uninitialized. Simply ignoring undefined sources for bit size
> >> validation is not enough since drivers can check and operate with the
> >> bit-size and that can lead to issues later on. Instead, fix undefined
> >> sources to always have a compatible bit size.
> >>
> >
> > I'm not sure what I think about this.  I think I'd rather have undefs
> > simply have the right bitsize.
> >
>
> With undefined sources you cannot get the bitsize from themselves
> because it is not initialized.

Doesn't patch 3 and the discussion on it imply that undefs should have
valid sizes?

> In that case, we pick the bit size from
> the ALU opcode's input definition. If it is unsized, then we use the
> destination size.

I dont think pulling the implicit size from the source size is ever
correct.  If you have an explicitly sized source that means it doesn't
affect and isn't affected by the implicit size.

> I think this is the right bitsize... or am I missing something?
>
> Sam
>
> >
> >> v2 (Sam):
> >> - Use helper to get type size from nir_alu_type.
> >> ---
> >>  src/compiler/nir/nir_validate.c | 10 ++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/src/compiler/nir/nir_validate.c
> >> b/src/compiler/nir/nir_validate.c
> >> index 9f18d1c..645c15a 100644
> >> --- a/src/compiler/nir/nir_validate.c
> >> +++ b/src/compiler/nir/nir_validate.c
> >> @@ -180,9 +180,11 @@ validate_alu_src(nir_alu_instr *instr, unsigned
> >> index, validate_state *state)
> >>
> >> unsigned num_components;
> >> unsigned src_bit_size;
> >> +   bool is_undef = false;
> >> if (src->src.is_ssa) {
> >>src_bit_size = src->src.ssa->bit_size;
> >>num_components = src->src.ssa->num_components;
> >> +  is_undef = src->src.ssa->parent_instr->type ==
> >> nir_instr_type_ssa_undef;
> >> } else {
> >>src_bit_size = src->src.reg.reg->bit_size;
> >>if (src->src.reg.reg->is_packed)
> >> @@ -205,12 +207,20 @@ validate_alu_src(nir_alu_instr *instr, unsigned
> >> index, validate_state *state)
> >>
> >> if (nir_alu_type_get_type_size(src_type)) {
> >>/* This source has an explicit bit size */
> >> +  if (is_undef) {
> >> + src_bit_size = nir_alu_type_get_type_size(src_type);
> >> + src->src.ssa->bit_size = src_bit_size;
> >> +  }
> >>assert(nir_alu_type_get_type_size(src_type) == src_bit_size);
> >> } else {
> >>if
> >> (!nir_alu_type_get_type_size(nir_op_infos[instr->op].output_type)) {
> >>   unsigned dest_bit_size =
> >>  instr->dest.dest.is_ssa ? instr->dest.dest.ssa.bit_size
> >>  :
instr->dest.dest.reg.reg->bit_size;
> >> + if (is_undef) {
> >> +src_bit_size = dest_bit_size;
> >> +src->src.ssa->bit_size = dest_bit_size;
> >> + }
> >>   assert(dest_bit_size == src_bit_size);
> >>}
> >> }
> >> --
> >> 2.5.0
> >>
> >> ___
> >> 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] [PATCH 19/29] nir: fix up bit sizes for undefined alu sources

[Samuel Iglesias Gonsálvez]

On 21/03/16 23:54, Jason Ekstrand wrote:
> On Mon, Mar 21, 2016 at 5:05 AM, Samuel Iglesias Gonsálvez <
> sigles...@igalia.com> wrote:
> 
>> From: Iago Toral Quiroga 
>>
>> Undefined sources in alu operations don't have a valid bit size because
>> they are uninitialized. Simply ignoring undefined sources for bit size
>> validation is not enough since drivers can check and operate with the
>> bit-size and that can lead to issues later on. Instead, fix undefined
>> sources to always have a compatible bit size.
>>
> 
> I'm not sure what I think about this.  I think I'd rather have undefs
> simply have the right bitsize.
> 

With undefined sources you cannot get the bitsize from themselves
because it is not initialized. In that case, we pick the bit size from
the ALU opcode's input definition. If it is unsized, then we use the
destination size.

I think this is the right bitsize... or am I missing something?

Sam

> 
>> v2 (Sam):
>> - Use helper to get type size from nir_alu_type.
>> ---
>>  src/compiler/nir/nir_validate.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/src/compiler/nir/nir_validate.c
>> b/src/compiler/nir/nir_validate.c
>> index 9f18d1c..645c15a 100644
>> --- a/src/compiler/nir/nir_validate.c
>> +++ b/src/compiler/nir/nir_validate.c
>> @@ -180,9 +180,11 @@ validate_alu_src(nir_alu_instr *instr, unsigned
>> index, validate_state *state)
>>
>> unsigned num_components;
>> unsigned src_bit_size;
>> +   bool is_undef = false;
>> if (src->src.is_ssa) {
>>src_bit_size = src->src.ssa->bit_size;
>>num_components = src->src.ssa->num_components;
>> +  is_undef = src->src.ssa->parent_instr->type ==
>> nir_instr_type_ssa_undef;
>> } else {
>>src_bit_size = src->src.reg.reg->bit_size;
>>if (src->src.reg.reg->is_packed)
>> @@ -205,12 +207,20 @@ validate_alu_src(nir_alu_instr *instr, unsigned
>> index, validate_state *state)
>>
>> if (nir_alu_type_get_type_size(src_type)) {
>>/* This source has an explicit bit size */
>> +  if (is_undef) {
>> + src_bit_size = nir_alu_type_get_type_size(src_type);
>> + src->src.ssa->bit_size = src_bit_size;
>> +  }
>>assert(nir_alu_type_get_type_size(src_type) == src_bit_size);
>> } else {
>>if
>> (!nir_alu_type_get_type_size(nir_op_infos[instr->op].output_type)) {
>>   unsigned dest_bit_size =
>>  instr->dest.dest.is_ssa ? instr->dest.dest.ssa.bit_size
>>  : instr->dest.dest.reg.reg->bit_size;
>> + if (is_undef) {
>> +src_bit_size = dest_bit_size;
>> +src->src.ssa->bit_size = dest_bit_size;
>> + }
>>   assert(dest_bit_size == src_bit_size);
>>}
>> }
>> --
>> 2.5.0
>>
>> ___
>> 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] [PATCH 19/29] nir: fix up bit sizes for undefined alu sources

[Jason Ekstrand]

On Mon, Mar 21, 2016 at 5:05 AM, Samuel Iglesias Gonsálvez <
sigles...@igalia.com> wrote:

> From: Iago Toral Quiroga 
>
> Undefined sources in alu operations don't have a valid bit size because
> they are uninitialized. Simply ignoring undefined sources for bit size
> validation is not enough since drivers can check and operate with the
> bit-size and that can lead to issues later on. Instead, fix undefined
> sources to always have a compatible bit size.
>

I'm not sure what I think about this.  I think I'd rather have undefs
simply have the right bitsize.


> v2 (Sam):
> - Use helper to get type size from nir_alu_type.
> ---
>  src/compiler/nir/nir_validate.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/src/compiler/nir/nir_validate.c
> b/src/compiler/nir/nir_validate.c
> index 9f18d1c..645c15a 100644
> --- a/src/compiler/nir/nir_validate.c
> +++ b/src/compiler/nir/nir_validate.c
> @@ -180,9 +180,11 @@ validate_alu_src(nir_alu_instr *instr, unsigned
> index, validate_state *state)
>
> unsigned num_components;
> unsigned src_bit_size;
> +   bool is_undef = false;
> if (src->src.is_ssa) {
>src_bit_size = src->src.ssa->bit_size;
>num_components = src->src.ssa->num_components;
> +  is_undef = src->src.ssa->parent_instr->type ==
> nir_instr_type_ssa_undef;
> } else {
>src_bit_size = src->src.reg.reg->bit_size;
>if (src->src.reg.reg->is_packed)
> @@ -205,12 +207,20 @@ validate_alu_src(nir_alu_instr *instr, unsigned
> index, validate_state *state)
>
> if (nir_alu_type_get_type_size(src_type)) {
>/* This source has an explicit bit size */
> +  if (is_undef) {
> + src_bit_size = nir_alu_type_get_type_size(src_type);
> + src->src.ssa->bit_size = src_bit_size;
> +  }
>assert(nir_alu_type_get_type_size(src_type) == src_bit_size);
> } else {
>if
> (!nir_alu_type_get_type_size(nir_op_infos[instr->op].output_type)) {
>   unsigned dest_bit_size =
>  instr->dest.dest.is_ssa ? instr->dest.dest.ssa.bit_size
>  : instr->dest.dest.reg.reg->bit_size;
> + if (is_undef) {
> +src_bit_size = dest_bit_size;
> +src->src.ssa->bit_size = dest_bit_size;
> + }
>   assert(dest_bit_size == src_bit_size);
>}
> }
> --
> 2.5.0
>
> ___
> 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] [PATCH 19/29] nir: fix up bit sizes for undefined alu sources

[Samuel Iglesias Gonsálvez]

From: Iago Toral Quiroga 

Undefined sources in alu operations don't have a valid bit size because
they are uninitialized. Simply ignoring undefined sources for bit size
validation is not enough since drivers can check and operate with the
bit-size and that can lead to issues later on. Instead, fix undefined
sources to always have a compatible bit size.

v2 (Sam):
- Use helper to get type size from nir_alu_type.
---
 src/compiler/nir/nir_validate.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/compiler/nir/nir_validate.c b/src/compiler/nir/nir_validate.c
index 9f18d1c..645c15a 100644
--- a/src/compiler/nir/nir_validate.c
+++ b/src/compiler/nir/nir_validate.c
@@ -180,9 +180,11 @@ validate_alu_src(nir_alu_instr *instr, unsigned index, 
validate_state *state)
 
unsigned num_components;
unsigned src_bit_size;
+   bool is_undef = false;
if (src->src.is_ssa) {
   src_bit_size = src->src.ssa->bit_size;
   num_components = src->src.ssa->num_components;
+  is_undef = src->src.ssa->parent_instr->type == nir_instr_type_ssa_undef;
} else {
   src_bit_size = src->src.reg.reg->bit_size;
   if (src->src.reg.reg->is_packed)
@@ -205,12 +207,20 @@ validate_alu_src(nir_alu_instr *instr, unsigned index, 
validate_state *state)
 
if (nir_alu_type_get_type_size(src_type)) {
   /* This source has an explicit bit size */
+  if (is_undef) {
+ src_bit_size = nir_alu_type_get_type_size(src_type);
+ src->src.ssa->bit_size = src_bit_size;
+  }
   assert(nir_alu_type_get_type_size(src_type) == src_bit_size);
} else {
   if (!nir_alu_type_get_type_size(nir_op_infos[instr->op].output_type)) {
  unsigned dest_bit_size =
 instr->dest.dest.is_ssa ? instr->dest.dest.ssa.bit_size
 : instr->dest.dest.reg.reg->bit_size;
+ if (is_undef) {
+src_bit_size = dest_bit_size;
+src->src.ssa->bit_size = dest_bit_size;
+ }
  assert(dest_bit_size == src_bit_size);
   }
}
-- 
2.5.0

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