[Mesa-dev] [PATCH] ra: Add ra_set_node_reg()

[Tom Stellard]

On Mon, Apr 25, 2011 at 09:18:40AM -0700, Eric Anholt wrote:
> On Sun, 24 Apr 2011 22:58:59 -0700, Tom Stellard <tstellar at gmail.com> wrote:
> > On Sun, Apr 24, 2011 at 02:00:40PM -0700, Eric Anholt wrote:
> > > On Tue, 19 Apr 2011 23:09:46 -0700, Tom Stellard <tstellar at gmail.com> wrote:
> > > > This function makes it possible to include input / payload registers in
> > > > the interference graph.
> > > 
> > > [...]
> > > 
> > > > +/**
> > > > + * This function allows a user to manually assign a register to a node.  This
> > > > + * is useful for nodes that belong to register classes that contain a very small
> > > > + * number of registers that are not likely to be allocated if they end up at
> > > > + * the bottom of the stack.
> > > > + */
> > > > +void
> > > > +ra_set_node_reg(struct ra_graph *g, unsigned int n, unsigned int reg)
> > > > +{
> > > > +   g->nodes[n].reg = reg;
> > > > +   g->nodes[n].in_stack = GL_FALSE;
> > > > +}
> > > > +
> > > 
> > > So, this problem could also be solved by creating a register class per
> > > payload register and including the nodes for the payload data in those
> > > classes, right?  However, I think you've got a good, safe solution for
> > > some common uses that doesn't cause the size of the allocator data to
> > > explode even more.
> > > 
> > 
> > In order to include the payload registers in the interference graph,
> > it is necessary to create classes for them *and* use ra_set_node_reg().
> > Simply creating classes for the payload registers won't work, because
> > if the payload nodes aren't the first nodes popped off the stack, then
> > the payload registers will be assigned to non-payload nodes before they
> > can be assigned to the payload nodes.  However, the payload nodes still
> > need to be assigned to a class, otherwise the allocator will crash during
> > the pq test.
> 
> Yeah, for our driver we have a class of the actual registers, then other
> classes for the weird things (pairs, aligned pairs, contiguous
> bigger-than-pairs) that conflict with those base registers, so I was
> reviewing on the assumption that those nodes would be of the base
> register class but with their particular base register set.

We have 19 different classes plus the classes for the input registers.
There is one class each for one, two, one + alpha, and two + alpha
channel writes so we can do swizzle packing.  Plus there is a class
for every possible combination of channel writes (e.g. XY, XZ, X,
etc.) for writemasks that we can't change due to constraints on some of
the instructions.

> 
> The hypothetical single-payload-register class nodes would never be
> trivially colorable until they have no edges to any classes that could
> interfere with their register, so they should be pushed to the stack
> after all nodes that could interfere with it.  Unless you're seeing
> optimistic coloring just arbitrarily pushing?  I'm interested in what
> you saw when trying that, as it might point to a serious bug.

I'm pretty sure the payload registers were only causing a problem with
the optimistic coloring.

-Tom
> 
> Given that your patch makes optimistic coloring more likely, even more
> reason to go with it, though.
>