[Mesa-dev] [PATCH v2 02/12] genxml: Preserve fields that share dword space with addresses.

[Rafael Antognolli]

On Wed, Jan 24, 2018 at 11:20:07AM +0200, Pohjolainen, Topi wrote:
> On Fri, Jan 19, 2018 at 11:54:37AM -0800, Rafael Antognolli wrote:
> > Some instructions contain fields that are either an address or a value
> > of some type based on the content of other fields, such as clear color
> > values vs address. That works fine if these fields are in the less
> > significant dword, the lower 32 bits of the address, because they get
> > OR'ed with the address. But if they are in the higher 32 bits, they get
> > discarded.
> > 
> > On Gen10 we have fields that share space with the higher 16 bits of the
> > address too. This commit makes sure those fields don't get discarded.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > ---
> >  src/intel/genxml/gen_pack_header.py | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/intel/genxml/gen_pack_header.py b/src/intel/genxml/gen_pack_header.py
> > index e6cea8646ff..e81695e2aea 100644
> > --- a/src/intel/genxml/gen_pack_header.py
> > +++ b/src/intel/genxml/gen_pack_header.py
> > @@ -486,11 +486,16 @@ class Group(object):
> >                  v_address = "v%d_address" % index
> >                  print("   const uint64_t %s =\n      __gen_combine_address(data, &dw[%d], values->%s, %s);" %
> >                        (v_address, index, dw.address.name + field.dim, v))
> > -                v = v_address
> > -
> > +                if len(dw.fields) > address_count:
> > +                    print("   dw[%d] = %s;" % (index, v_address))
> > +                    print("   dw[%d] = (%s >> 32) | (%s >> 32);" % (index + 1, v_address, v))
> > +                    continue
> > +                else:
> > +                    v = v_address
> >              print("   dw[%d] = %s;" % (index, v))
> >              print("   dw[%d] = %s >> 32;" % (index + 1, v))
> 
> I'm wondering if we could have left the "continue" out and write the
> else-branch directly just like we did if:
> 
>                print("   dw[%d] = %s;" % (index, v_address))
>                print("   dw[%d] = %s >> 32;" % (index + 1, v_address))

Hi Topi,

I was rebasing the series on top of master and while applying your
suggestion, I just noticed it's not gonna work. Notice that the last 2
lines are executed both when the else branch is taken, or when the outer
"if dw.address:" is not taken. If I do as you suggest, that last case
won't be covered.

I know it looks really ugly, I'll try to think of a better way to do
this, but for now I'll just submit the updated series with this version
again to get reviews on other stuff.

Thanks for the review anyway.

> >  
> > +
> >  class Value(object):
> >      def __init__(self, attrs):
> >          self.name = safe_name(attrs["name"])
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev