Attachment 105645 Details for Bug 82828 – proposed fix

[patch] proposed fix

0001-r300g-set-register-classes-before-interferences.patch (text/plain), 2.69 KB, created by Connor Abbott on 2014-09-03 00:26:07 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Connor Abbott

Created: 2014-09-03 00:26:07 UTC

Size: 2.69 KB

patch

obsolete

>From b28eb9d4a0b28f4a28afa08a64063be492532041 Mon Sep 17 00:00:00 2001
>From: Connor Abbott <cwabbott0@gmail.com>
>Date: Tue, 2 Sep 2014 20:14:56 -0400
>Subject: [PATCH] r300g: set register classes before interferences
>
>In commit 567e2769b81863b6dffdac3826a6b729ce6ea37c ("ra: make the p, q
>test more efficient") I unknowingly introduced a new requirement to the
>register allocator API: the user must set the register class of all
>nodes before setting up their interferences, because
>ra_add_conflict_list() now uses the classes of the two interfering
>nodes. i965 already did this, but r300g was setting up register classes
>interleaved with setting up the interference graph. This led to us
>calculating the wrong q total, and in certain cases
>e78a01d5e6f77e075fe667a0f0ccb10d89c0dd58 (" ra: optimistically color
>only one node at a time") made it so that this bug caused a segfault. In
>particular, the error occurred if the q total was decremented to 1 below
>0 for the last node to be pushed onto the stack.  Since q_total is an
>unsigned integer, it overflowed to 0xffffffff, which is what
>lowest_q_total happens to be initialzed to. This means that we would
>fail the "new_q_total < lowest_q_total" check on line 476 of
>register_allocate.c, and so the node would never be pushed onto the
>stack, which led to segfaults in ra_select() when we failed to ever give
>it a register.
>
>Signed-off-by: Connor Abbott <cwabbott0@gmail.com>
>---
>I think we could get rid of the node_classes array if we call 
>ra_alloc_interference_graph() earlier, but I don't know much about this 
>code so I just did the minimum amount to fix it.
> src/gallium/drivers/r300/compiler/radeon_pair_regalloc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/src/gallium/drivers/r300/compiler/radeon_pair_regalloc.c b/src/gallium/drivers/r300/compiler/radeon_pair_regalloc.c
>index 936c88d..b854a2f 100644
>--- a/src/gallium/drivers/r300/compiler/radeon_pair_regalloc.c
>+++ b/src/gallium/drivers/r300/compiler/radeon_pair_regalloc.c
>@@ -572,14 +572,16 @@ static void do_advanced_regalloc(struct regalloc_state * s)
> 	graph = ra_alloc_interference_graph(ra_state->regs,
> 						node_count + s->NumInputs);
> 
>+	for (node_index = 0; node_index < node_count; node_index++) {
>+		ra_set_node_class(graph, node_index, node_classes[node_index]);
>+	}
>+
> 	/* Build the interference graph */
> 	for (var_ptr = variables, node_index = 0; var_ptr;
> 					var_ptr = var_ptr->Next,node_index++) {
> 		struct rc_list * a, * b;
> 		unsigned int b_index;
> 
>-		ra_set_node_class(graph, node_index, node_classes[node_index]);
>-
> 		for (a = var_ptr, b = var_ptr->Next, b_index = node_index + 1;
> 						b; b = b->Next, b_index++) {
> 			struct rc_variable * var_a = a->Item;
>-- 
>1.8.5.2 (Apple Git-48)
>

Actions: View | Splinter Review

Attachments on bug 82828: 104921 | 105451 | 105572 | 105630 | 105641 | 105645

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.