[Mesa-dev] [PATCH] r600g/sb: fix stack size computation on evergreen

[Tom Stellard]

On Tue, Dec 10, 2013 at 02:54:46AM +0400, Vadim Girlin wrote:
> On Mon, 2013-12-09 at 10:56 -0500, Tom Stellard wrote:
> > On Sat, Dec 07, 2013 at 07:06:36PM +0400, Vadim Girlin wrote:
> > > On evergreen we have to reserve 1 stack element in some additional cases
> > > besides the ones mentioned in the docs, but stack size computation was
> > > recently reimplemented exactly as described in the docs by the patch that
> > > added workarounds for stack issues on EG/CM, resulting in regressions
> > > with some apps (Serious Sam 3).
> > > 
> > > This patch fixes it by restoring previous behavior.
> > > 
> > > Fixes https://bugs.freedesktop.org/show_bug.cgi?id=72369
> > > 
> > > Signed-off-by: Vadim Girlin <vadimgirlin at gmail.com>
> > > Cc: "10.0" <mesa-stable at lists.freedesktop.org>
> > > ---
> > >  src/gallium/drivers/r600/sb/sb_bc_finalize.cpp | 16 ++++++++++++----
> > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> > > index bc71cf8..355eb63 100644
> > > --- a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> > > +++ b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> > > @@ -770,7 +770,6 @@ void bc_finalizer::update_ngpr(unsigned gpr) {
> > >  unsigned bc_finalizer::get_stack_depth(node *n, unsigned &loops,
> > >                                             unsigned &ifs, unsigned add) {
> > >  	unsigned stack_elements = add;
> > > -	bool has_non_wqm_push_with_loops_on_stack = false;
> > >  	bool has_non_wqm_push = (add != 0);
> > >  	region_node *r = n->is_region() ?
> > >  			static_cast<region_node*>(n) : n->get_parent_region();
> > > @@ -781,8 +780,6 @@ unsigned bc_finalizer::get_stack_depth(node *n, unsigned &loops,
> > >  	while (r) {
> > >  		if (r->is_loop()) {
> > >  			++loops;
> > > -			if (has_non_wqm_push)
> > > -				has_non_wqm_push_with_loops_on_stack = true;
> > >  		} else {
> > >  			++ifs;
> > >  			has_non_wqm_push = true;
> > > @@ -795,15 +792,26 @@ unsigned bc_finalizer::get_stack_depth(node *n, unsigned &loops,
> > >  	switch (ctx.hw_class) {
> > >  	case HW_CLASS_R600:
> > >  	case HW_CLASS_R700:
> > > +		// If any non-WQM push is invoked, 2 elements should be reserved.
> > >  		if (has_non_wqm_push)
> > >  			stack_elements += 2;
> > >  		break;
> > >  	case HW_CLASS_CAYMAN:
> > > +		// If any stack operation is invoked, 2 elements should be reserved
> > >  		if (stack_elements)
> > >  			stack_elements += 2;
> > >  		break;
> > >  	case HW_CLASS_EVERGREEN:
> > > -		if (has_non_wqm_push_with_loops_on_stack)
> > > +		// According to the docs we need to reserve 1 element for each of the
> > > +		// following cases:
> > > +		//   1) non-WQM push is used with WQM/LOOP frames on stack
> > > +		//   2) ALU_ELSE_AFTER is used at the point of max stack usage
> > > +		// NOTE:
> > > +		// It was found that the conditions above are not sufficient, there are
> > > +		// other cases where we also need to reserve stack space, that's why
> > > +		// we always reserve 1 stack element if we have non-WQM push on stack.
> > > +		// Condition 2 is ignored for now because we don't use this instruction.
> > > +		if (has_non_wqm_push)
> > >  			++stack_elements;
> > 
> > The kernel analyzer reports a stack size of 2 for compute shaders that
> > have 3 levels of ALU_PUSH_BEFORE.  This would suggest that you either need to
> > reserve 2 sub-entries (stack_elements in the sb code) when there is a
> > non-wqm push, or apply the CAYMAN rules to EVERGREEN.
> > 
> > It is possible, though, that the kernel analyzer is over-allocating and
> > this patch is correct, but I don't have any evidence for this yet.
> 
> Is there any test that fails with this patch? AFAIK this algorithm
> worked fine for about 8 months in both old and sb backends, so I'd
> rather prefer to have any evidence that this is not correct before
> increasing stack allocation and reducing performance.
>
> Vadim

I have not been able to find a failing test when reserving one extra
element for the first non-wqm push (this includes a full run of the
OpenCV OpenCL test-suite which has a lot of complex control flow),
so I think the way you've implemented it in this patch is safe.

>From analyzing dumps for the kernel analyzer, it looks like they count
two extra sub-entries for the first non-wqm push when calculating the
stack size, but they count only one extra sub-entry when determining
whether or not to apply the work-around for the HW bug.  This also makes
me think that allocating one extra sub-entry is probably correct.

-Tom

> 
> > 
> > -Tom
> > 
> > 
> > >  		break;
> > >  	}
> > > -- 
> > > 1.8.4.2
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
>