Register allocator and lexicals bug #262

Closed
jnthn opened this Issue Apr 7, 2011 · 4 comments

Comments

Projects
None yet
3 participants
@jnthn
Contributor

jnthn commented Apr 7, 2011

Code:

.sub ''
$P42 = new ['Integer']
.lex 'i', $P42
$P69 = new ['Integer']
.lex 'j', $P69
.lex 'foo', $P10
.lex 'bar', $P11
.lex 'baz', $P12
$P3 = new ['String']
.lex 'k', $P3

$P100 = new ['String']
$P100 = "oh no"
store_lex 'foo', $P100
$P101 = find_lex 'bar'
say $P101
.end

Should be a null PMC access, but prints "oh no". It looks like

.lex 'foo', $P0

isn't considered an instruction, but just saying "this lexical goes in this register". If that's the only mention of $P0, then there's no "first instruction". It builds a du-chain, then based on that throws away $P0 from the "needs allocation" list because it confuses it with an unused register (e.g. it thinks it got optimized out, which is not the case at all here).

This patch makes the issue go away:

diff --git a/compilers/imcc/reg_alloc.c b/compilers/imcc/reg_alloc.c
index de0f3e0..300216d 100644
--- a/compilers/imcc/reg_alloc.c
+++ b/compilers/imcc/reg_alloc.c
@@ -391,6 +391,9 @@ reg_sort_f(ARGIN(const void *a), ARGIN(const void *b))
     const SymReg * const ra = *(const SymReg * const *)a;
     const SymReg * const rb = *(const SymReg * const *)b;

+    if (!ra->first_ins || !rb->first_ins)
+        return 0;
+    
     if (ra->first_ins->index < rb->first_ins->index)
         return -1;

@@ -473,7 +476,7 @@ build_reglist(PARROT_INTERP, ARGMOD(IMC_Unit *unit))

     /* we might have unused symbols here, from optimizations */
     for (i = count = unused = 0; i < n_symbols; i++) {
-        if (!unit->reglist[i]->first_ins)
+        if (!unit->reglist[i]->first_ins && !(unit->reglist[i]->usage & U_LEXICAL))
             unused++;
         else if (i == count)
             count++;
@@ -716,7 +719,7 @@ vanilla_reg_alloc(PARROT_INTERP, ARGMOD(IMC_Unit *unit))
         SymReg *r;
         for (r = hsh->data[i]; r; r = r->next) {
             /* TODO Ignore non-volatiles */
-            if (REG_NEEDS_ALLOC(r) && r->use_count)
+            if (REG_NEEDS_ALLOC(r))
                 r->color = -1;
         }
     }
@@ -732,7 +735,7 @@ vanilla_reg_alloc(PARROT_INTERP, ARGMOD(IMC_Unit *unit))
             for (r = hsh->data[i]; r; r = r->next) {
                 if (r->set != reg_set)
                     continue;
-                if (REG_NEEDS_ALLOC(r) && (r->color == -1) && r->use_count) {
+                if (REG_NEEDS_ALLOC(r) && (r->color == -1)) {
                     if (set_contains(avail, first_reg))
                         first_reg = first_avail(interp, unit, reg_set, NULL);

But it's kinda hacky. OTOH, so is the register allocator as a whole, and this is blocking me on NQP, so provided we can be sure we don't make the quicksort of symbols to allocate unstable I'd be OK with something like this going in. Will leave it until tomorrow to see if anyone has any better ideas.

Originally http://trac.parrot.org/parrot/ticket/2087

@leto

This comment has been minimized.

Show comment
Hide comment
@leto

leto Apr 28, 2011

Member

Is this still blocking you, Jonathan? If parrot fulltest and rakudo spectest pass with this patch, then we should apply it, barring horrible performance problems.

Member

leto commented Apr 28, 2011

Is this still blocking you, Jonathan? If parrot fulltest and rakudo spectest pass with this patch, then we should apply it, barring horrible performance problems.

@ghost ghost assigned Whiteknight Feb 22, 2012

@Whiteknight

This comment has been minimized.

Show comment
Hide comment
@Whiteknight

Whiteknight May 7, 2012

Contributor

I just tried to apply this patch and it doesn't apply cleanly. After a quick visual inspection, it appears that most of this patch has already been applied (and the rest I either can't find or it isn't applied). Running the test program program provided does indeed fail with a null pmc access which, I think is what you said was expected.

@jnthn, Is this issue resolved? If so, can we close this ticket? Thanks.

Contributor

Whiteknight commented May 7, 2012

I just tried to apply this patch and it doesn't apply cleanly. After a quick visual inspection, it appears that most of this patch has already been applied (and the rest I either can't find or it isn't applied). Running the test program program provided does indeed fail with a null pmc access which, I think is what you said was expected.

@jnthn, Is this issue resolved? If so, can we close this ticket? Thanks.

@jnthn

This comment has been minimized.

Show comment
Hide comment
@jnthn

jnthn May 8, 2012

Contributor

On 5/8/2012 0:58, Andrew Whitworth wrote:

I just tried to apply this patch and it doesn't apply cleanly. After a quick visual inspection, it appears that most of this patch has already been applied (and the rest I either can't find or it isn't applied). Running the test program program provided does indeed fail with a null pmc access which, I think is what you said was expected.

@jnthn, Is this issue resolved? If so, can we close this ticket? Thanks.
I recall getting the fix for this in to Parrot, and haven't seen any
allocator problems of this nature since (we'd be pretty hosed in Rakudo
if this was still broken). So, can close.

/jnthn

Contributor

jnthn commented May 8, 2012

On 5/8/2012 0:58, Andrew Whitworth wrote:

I just tried to apply this patch and it doesn't apply cleanly. After a quick visual inspection, it appears that most of this patch has already been applied (and the rest I either can't find or it isn't applied). Running the test program program provided does indeed fail with a null pmc access which, I think is what you said was expected.

@jnthn, Is this issue resolved? If so, can we close this ticket? Thanks.
I recall getting the fix for this in to Parrot, and haven't seen any
allocator problems of this nature since (we'd be pretty hosed in Rakudo
if this was still broken). So, can close.

/jnthn

@Whiteknight

This comment has been minimized.

Show comment
Hide comment
@Whiteknight

Whiteknight May 9, 2012

Contributor

Thanks @jnthn. I'm closing this ticket. Let us know if you find any other problems.

Contributor

Whiteknight commented May 9, 2012

Thanks @jnthn. I'm closing this ticket. Let us know if you find any other problems.

@Whiteknight Whiteknight closed this May 9, 2012

@Whiteknight Whiteknight removed their assignment Mar 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment