This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Slightly decrease .debug_loc size
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Jason Merrill <jason at redhat dot com>
- Cc: Richard Guenther <rguenther at suse dot de>, Alexandre Oliva <aoliva at redhat dot com>, gcc-patches at gcc dot gnu dot org
- Date: Tue, 20 Apr 2010 18:43:58 +0200
- Subject: Re: [PATCH] Slightly decrease .debug_loc size
- References: <20100414180534.GB2817@tyan-ft48-01.lab.bos.redhat.com> <4BCDD2F0.6020403@redhat.com>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
On Tue, Apr 20, 2010 at 12:14:40PM -0400, Jason Merrill wrote:
> On 04/14/2010 02:05 PM, Jakub Jelinek wrote:
> >+ /* If the last note doesn't cover any instructions, remove it. */
>
> This may be the abstract effect, but it doesn't really describe what
> the code is doing. I think it would be clearer to say "if there
> haven't been any new instructions since the last node, reuse it."
The code removes the last note if it doesn't affect any instructions.
Then, either it creates a new note (and then can reuse the storage instead
of ggc_freeing it and allocating it immediately again), or removes it.
> >- temp->last = loc;
> >...
> >+ if (last != temp->last)
> >+ temp->last = last;
>
> Why is this changed? It seems like this change means that
> temp->last and temp->last->next will be different where before they
> were always set to the same value.
temp->last should point to one before last node if possible (more than one
node in the chain) and it is possible the last node might be redundant
(not affecting any real insns). If last here is a pointer to the last
node after optional removal of one node but before addition of the new node,
temp->last is the pointer we had. If last == temp->last, then we don't want
to change temp->last (it now points to one before last node after the
addition). If last != temp->last, then temp->last pointed to one before
last node before the addition, so no points to second node before last
and thus in the next iteration gcc_assert (last->next == NULL); would
fail. It is enough to point to one before last, if we didn't remove the
note as unnecessary until now, we won't remove it ever.
> >+ else if (unused)
> >+ ggc_free (unused);
>
> So in this case we're just removing the last entry from the location
> list and not replacing it with anything? That can't be right.
Consider two cases:
(note ... (var_location i (reg:SI 5 di [ i ])) NOTE_INSN_VAR_LOCATION)
some real insns
(note ... (var_location i (nil)) NOTE_INSN_VAR_LOCATION)
just other notes
(note ... (var_location i (reg:SI 5 di [ i ])) NOTE_INSN_VAR_LOCATION)
and
(note ... (var_location i (reg:SI 5 di [ i ])) NOTE_INSN_VAR_LOCATION)
some real insns
(note ... (var_location i (nil)) NOTE_INSN_VAR_LOCATION)
just other notes
(note ... (var_location i (reg:SI 0 ax [ i ])) NOTE_INSN_VAR_LOCATION)
In both cases the i/(nil) note doesn't affect any real instructions,
so the new code removes it. In the first case then the new last note
after the removal of the useless one (i/di) has the same location and init
status as the current note, so in that case we don't want to reuse it,
just return that we don't want to do anything (the last i/di note is still
valid). In the second case the location is different and thus we need to
create a new location in the chain (and reuse for that the storage we
haven't ggc_freed yet).
>
> >+ /* If there were no real insns between note we processed last time
> >+ and this note, use the label we emitted last time. */
> >+ if (last_var_location_insn == NULL_RTX
> >+ || last_var_location_insn != next_real
> >+ || last_in_cold_section_p != in_cold_section_p)
> >+ {
> >+ last_label = NULL;
> >+ last_postcall_label = NULL;
> >+ }
>
> This comment seems to describe what eventually happens if the
> condition is false; I think it would be more helpful for it to
> address what's actually happening in this code.
Ok, will change that.
Jakub