Hi,
[FWIW I can't approve patches, but some feedback nevertheless]
On Sun, 17 Apr 2011, Easwaran Raman wrote:
> This patch impoves the heuristic used in assigning stack location to
> stack variables. Currently, if there are 3 variables A, B and C with
> their sizes in increasing order and A and C have a conflict, it will
> put A and B in a partition and C in a separate partition with a total
> frame size of sizeof(B) + sizeof(C). This patch puts B and C in the
> same partition and A in a separate partition, with a total size of
> sizeof(A) + sizeof(C).
That's the change in stack_var_cmp, to match the comment, right? Makes
sense.
> The other change in this patch removes the field offset in struct
> stack_var. Right now, the field is always 0 due to the way it is set in
> partition_stack_vars.
Huh, indeed, starting with the initial version of that code already. The
idea clearly was to pack multiple conflicting small objects into the space
of only one larger non-conflicting one by placing them at different
offsets, but that never seems to have been implemented and would require
different tracking of conflicts. I think removing the offset tracking
right now is okay, can be reintroduced once somebody gets motivated
enough.
> - and merge them into partition A. */
> - for (last = i = b; i != EOC; last = i, i = stack_vars[i].next)
> - {
> - stack_vars[i].offset += offset;
> - stack_vars[i].representative = a;
> - }
> - stack_vars[last].next = stack_vars[a].next;
> + /* Add B to A's partition. */
> + stack_vars[b].next = stack_vars[a].next;
> + stack_vars[b].representative = a;
Hmm. This seems fishy. You don't update the representatives of the
members of the partition that B is the leader of. Additionally you break
the chain of B's members. That is only a problem if B actually has more
than one member. That might be always the case with your changed sorting
order, but it's an important invariant, so please assert this in this
routine.
> @@ -596,7 +581,7 @@
> if (vb->conflicts)
> {
> EXECUTE_IF_SET_IN_BITMAP (vb->conflicts, 0, u, bi)
> - add_stack_var_conflict (a, stack_vars[u].representative);
> + add_stack_var_conflict (a, u);
Please don't. This uselessly bloats the conflict bitmaps.
Ciao,
Michael.
