[03/10] intel/fs: Fix bug in lower_simd_width while splitting an instruction which was already split.

Submitted by Francisco Jerez on Dec. 29, 2018, 8:38 p.m.

Details

Message ID 20181229203904.18847-4-currojerez@riseup.net
State New
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Francisco Jerez Dec. 29, 2018, 8:38 p.m.
This seems to be a problem in combination with the lower_regioning
pass introduced by a future commit, which can modify a SIMD-split
instruction causing its execution size to become illegal again.  A
subsequent call to lower_simd_width() would hit this bug on a future
platform.

Cc: mesa-stable@lists.freedesktop.org
---
 src/intel/compiler/brw_fs.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
index 97544fdf465..4aacc72a1b7 100644
--- a/src/intel/compiler/brw_fs.cpp
+++ b/src/intel/compiler/brw_fs.cpp
@@ -5666,7 +5666,7 @@  static fs_reg
 emit_unzip(const fs_builder &lbld, fs_inst *inst, unsigned i)
 {
    /* Specified channel group from the source region. */
-   const fs_reg src = horiz_offset(inst->src[i], lbld.group());
+   const fs_reg src = horiz_offset(inst->src[i], lbld.group() - inst->group);
 
    if (needs_src_copy(lbld, inst, i)) {
       /* Builder of the right width to perform the copy avoiding uninitialized
@@ -5757,7 +5757,7 @@  emit_zip(const fs_builder &lbld_before, const fs_builder &lbld_after,
    assert(lbld_before.group() == lbld_after.group());
 
    /* Specified channel group from the destination region. */
-   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group());
+   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group() - inst->group);
    const unsigned dst_size = inst->size_written /
       inst->dst.component_size(inst->exec_size);
 

Comments

On Sat, 2018-12-29 at 12:38 -0800, Francisco Jerez wrote:
> This seems to be a problem in combination with the lower_regioning
> pass introduced by a future commit, which can modify a SIMD-split
> instruction causing its execution size to become illegal again.  A
> subsequent call to lower_simd_width() would hit this bug on a future
> platform.
> 
> Cc: mesa-stable@lists.freedesktop.org
> ---
>  src/intel/compiler/brw_fs.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index 97544fdf465..4aacc72a1b7 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -5666,7 +5666,7 @@ static fs_reg
>  emit_unzip(const fs_builder &lbld, fs_inst *inst, unsigned i)
>  {
>     /* Specified channel group from the source region. */
> -   const fs_reg src = horiz_offset(inst->src[i], lbld.group());
> +   const fs_reg src = horiz_offset(inst->src[i], lbld.group() -
> inst->group);

Should we assert that lbld.group >= inst->group? Same below.

>     if (needs_src_copy(lbld, inst, i)) {
>        /* Builder of the right width to perform the copy avoiding
> uninitialized
> @@ -5757,7 +5757,7 @@ emit_zip(const fs_builder &lbld_before, const
> fs_builder &lbld_after,
>     assert(lbld_before.group() == lbld_after.group());
>  
>     /* Specified channel group from the destination region. */
> -   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group());
> +   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group() -
> inst->group);
>     const unsigned dst_size = inst->size_written /
>        inst->dst.component_size(inst->exec_size);
>
Iago Toral <itoral@igalia.com> writes:

> On Sat, 2018-12-29 at 12:38 -0800, Francisco Jerez wrote:
>> This seems to be a problem in combination with the lower_regioning
>> pass introduced by a future commit, which can modify a SIMD-split
>> instruction causing its execution size to become illegal again.  A
>> subsequent call to lower_simd_width() would hit this bug on a future
>> platform.
>> 
>> Cc: mesa-stable@lists.freedesktop.org
>> ---
>>  src/intel/compiler/brw_fs.cpp | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/intel/compiler/brw_fs.cpp
>> b/src/intel/compiler/brw_fs.cpp
>> index 97544fdf465..4aacc72a1b7 100644
>> --- a/src/intel/compiler/brw_fs.cpp
>> +++ b/src/intel/compiler/brw_fs.cpp
>> @@ -5666,7 +5666,7 @@ static fs_reg
>>  emit_unzip(const fs_builder &lbld, fs_inst *inst, unsigned i)
>>  {
>>     /* Specified channel group from the source region. */
>> -   const fs_reg src = horiz_offset(inst->src[i], lbld.group());
>> +   const fs_reg src = horiz_offset(inst->src[i], lbld.group() -
>> inst->group);
>
> Should we assert that lbld.group >= inst->group? Same below.
>

The IR will fail validation anytime that's not the case.  But I can add
the assertions in both places if that makes you feel more comfortable.

>>     if (needs_src_copy(lbld, inst, i)) {
>>        /* Builder of the right width to perform the copy avoiding
>> uninitialized
>> @@ -5757,7 +5757,7 @@ emit_zip(const fs_builder &lbld_before, const
>> fs_builder &lbld_after,
>>     assert(lbld_before.group() == lbld_after.group());
>>  
>>     /* Specified channel group from the destination region. */
>> -   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group());
>> +   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group() -
>> inst->group);
>>     const unsigned dst_size = inst->size_written /
>>        inst->dst.component_size(inst->exec_size);
>>
On Mon, 2019-01-07 at 11:58 -0800, Francisco Jerez wrote:
> Iago Toral <itoral@igalia.com> writes:
> 
> > On Sat, 2018-12-29 at 12:38 -0800, Francisco Jerez wrote:
> > > This seems to be a problem in combination with the
> > > lower_regioning
> > > pass introduced by a future commit, which can modify a SIMD-split
> > > instruction causing its execution size to become illegal
> > > again.  A
> > > subsequent call to lower_simd_width() would hit this bug on a
> > > future
> > > platform.
> > > 
> > > Cc: mesa-stable@lists.freedesktop.org
> > > ---
> > >  src/intel/compiler/brw_fs.cpp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/intel/compiler/brw_fs.cpp
> > > b/src/intel/compiler/brw_fs.cpp
> > > index 97544fdf465..4aacc72a1b7 100644
> > > --- a/src/intel/compiler/brw_fs.cpp
> > > +++ b/src/intel/compiler/brw_fs.cpp
> > > @@ -5666,7 +5666,7 @@ static fs_reg
> > >  emit_unzip(const fs_builder &lbld, fs_inst *inst, unsigned i)
> > >  {
> > >     /* Specified channel group from the source region. */
> > > -   const fs_reg src = horiz_offset(inst->src[i], lbld.group());
> > > +   const fs_reg src = horiz_offset(inst->src[i], lbld.group() -
> > > inst->group);
> > 
> > Should we assert that lbld.group >= inst->group? Same below.
> > 
> 
> The IR will fail validation anytime that's not the case.  But I can
> add
> the assertions in both places if that makes you feel more
> comfortable.

I guess you are referring to this assert at codegen time:

assert(inst->force_writemask_all || inst->group % inst->exec_size ==
0);

I guess that is probably enough, but I would still prefer to add the
asserts here too if that's okay.

> > >     if (needs_src_copy(lbld, inst, i)) {
> > >        /* Builder of the right width to perform the copy avoiding
> > > uninitialized
> > > @@ -5757,7 +5757,7 @@ emit_zip(const fs_builder &lbld_before,
> > > const
> > > fs_builder &lbld_after,
> > >     assert(lbld_before.group() == lbld_after.group());
> > >  
> > >     /* Specified channel group from the destination region. */
> > > -   const fs_reg dst = horiz_offset(inst->dst,
> > > lbld_after.group());
> > > +   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group()
> > > -
> > > inst->group);
> > >     const unsigned dst_size = inst->size_written /
> > >        inst->dst.component_size(inst->exec_size);
> > >
Iago Toral <itoral@igalia.com> writes:

> On Mon, 2019-01-07 at 11:58 -0800, Francisco Jerez wrote:
>> Iago Toral <itoral@igalia.com> writes:
>> 
>> > On Sat, 2018-12-29 at 12:38 -0800, Francisco Jerez wrote:
>> > > This seems to be a problem in combination with the
>> > > lower_regioning
>> > > pass introduced by a future commit, which can modify a SIMD-split
>> > > instruction causing its execution size to become illegal
>> > > again.  A
>> > > subsequent call to lower_simd_width() would hit this bug on a
>> > > future
>> > > platform.
>> > > 
>> > > Cc: mesa-stable@lists.freedesktop.org
>> > > ---
>> > >  src/intel/compiler/brw_fs.cpp | 4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > > 
>> > > diff --git a/src/intel/compiler/brw_fs.cpp
>> > > b/src/intel/compiler/brw_fs.cpp
>> > > index 97544fdf465..4aacc72a1b7 100644
>> > > --- a/src/intel/compiler/brw_fs.cpp
>> > > +++ b/src/intel/compiler/brw_fs.cpp
>> > > @@ -5666,7 +5666,7 @@ static fs_reg
>> > >  emit_unzip(const fs_builder &lbld, fs_inst *inst, unsigned i)
>> > >  {
>> > >     /* Specified channel group from the source region. */
>> > > -   const fs_reg src = horiz_offset(inst->src[i], lbld.group());
>> > > +   const fs_reg src = horiz_offset(inst->src[i], lbld.group() -
>> > > inst->group);
>> > 
>> > Should we assert that lbld.group >= inst->group? Same below.
>> > 
>> 
>> The IR will fail validation anytime that's not the case.  But I can
>> add
>> the assertions in both places if that makes you feel more
>> comfortable.
>
> I guess you are referring to this assert at codegen time:
>
> assert(inst->force_writemask_all || inst->group % inst->exec_size ==
> 0);
>
> I guess that is probably enough, but I would still prefer to add the
> asserts here too if that's okay.
>

Nah, I was thinking of the i965 IR validator that checks for
out-of-bounds VGRF register accesses.  But the asserts would be more
strict -- Just added them locally.

Thanks!

>> > >     if (needs_src_copy(lbld, inst, i)) {
>> > >        /* Builder of the right width to perform the copy avoiding
>> > > uninitialized
>> > > @@ -5757,7 +5757,7 @@ emit_zip(const fs_builder &lbld_before,
>> > > const
>> > > fs_builder &lbld_after,
>> > >     assert(lbld_before.group() == lbld_after.group());
>> > >  
>> > >     /* Specified channel group from the destination region. */
>> > > -   const fs_reg dst = horiz_offset(inst->dst,
>> > > lbld_after.group());
>> > > +   const fs_reg dst = horiz_offset(inst->dst, lbld_after.group()
>> > > -
>> > > inst->group);
>> > >     const unsigned dst_size = inst->size_written /
>> > >        inst->dst.component_size(inst->exec_size);
>> > >