On Wed, Feb 21, 2018 at 11:11 AM, Alexandre Oliva <[email protected]> wrote:
> On Feb 15, 2018, Szabolcs Nagy <[email protected]> wrote:
>
>> i see assembler slow downs with these location view patches
>> i opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84408
>
>
> [LVU] reset view at function entry, omit views at line zero
>
> Location views might be associated with locations that lack line
> number information (line number zero), but since we omit .loc
> directives that would have been issued with line number zero, we also
> omit the symbolic view numbers that would have been issued at such
> points.
>
> Resetting views at function entry points address some of these issues,
> and alleviate the huge chains of symbolic views that have burdened
> assemblers since we disabled -ginternal-reset-location-views by
> default, but other problems of undefined views remain when it's not
> the whole function that lacks line number info, just parts of it.
>
> So, when we encounter a request to output a view that may have been
> referenced, but we decide to omit the .loc because the line is zero,
> we will now omit the view as well, i.e., we will internally regard
> that view as zero-numbered.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu. Ok to install?
>
> Uros, could you please confirm whether this fixes the 84404 go problem
> you reported on alpha? I'm guessing it's the same issue. TIA,
I can confirm, that all "leb128 operand is an undefined symbol" errors
are gone. I'm running full alpha native bootstrap & regression test,
but I don't expect any surprises here.
Thanks,
Uros.
> for gcc/ChangeLog
>
> PR debug/84404
> PR debug/84408
> * dwarf2out.c (struct dw_line_info_table): Update comments for
> view == -1.
> (FORCE_RESET_NEXT_VIEW): New.
> (FORCE_RESETTING_VIEW_P): New.
> (RESETTING_VIEW_P): Check for -1 too.
> (ZERO_VIEW_P): Likewise.
> (new_line_info_table): Force-reset next view.
> (dwarf2out_begin_function): Likewise.
> (dwarf2out_source_line): Simplify zero_view_p initialization.
> Test FORCE_RESETTING_VIEW_P and RESETTING_VIEW_P instead of
> view directly. Omit view when omitting .loc at line 0.
>
> for gcc/testsuite/ChangeLog
>
> PR debug/84404
> PR debug/84408
> * gcc.dg/graphite/pr84404.c: New.
> ---
> gcc/dwarf2out.c | 89
> ++++++++++++++++++++++++-------
> gcc/testsuite/gcc.dg/graphite/pr84404.c | 18 ++++++
> 2 files changed, 87 insertions(+), 20 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/graphite/pr84404.c
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 5e88c7bacf06..7bbe20e495ac 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -2940,8 +2940,8 @@ struct GTY(()) dw_line_info_table {
> If it is 0, it is known that the NEXT view will be the first view
> at the given PC.
>
> - If it is -1, we've advanced PC but we haven't emitted a line location
> yet,
> - so we shouldn't use this view number.
> + If it is -1, we're forcing the view number to be reset, e.g. at a
> + function entry.
>
> The meaning of other nonzero values depends on whether we're
> computing views internally or leaving it for the assembler to do
> @@ -2951,8 +2951,10 @@ struct GTY(()) dw_line_info_table {
> going to ask the assembler to assign. */
> var_loc_view view;
>
> +#define FORCE_RESET_NEXT_VIEW(x) ((x) = (var_loc_view)-1)
> #define RESET_NEXT_VIEW(x) ((x) = (var_loc_view)0)
> -#define RESETTING_VIEW_P(x) ((x) == (var_loc_view)0)
> +#define FORCE_RESETTING_VIEW_P(x) ((x) == (var_loc_view)-1)
> +#define RESETTING_VIEW_P(x) ((x) == (var_loc_view)0 ||
> FORCE_RESETTING_VIEW_P (x))
>
> vec<dw_line_info_entry, va_gc> *entries;
> };
> @@ -2985,7 +2987,7 @@ maybe_reset_location_view (rtx_insn *insn,
> dw_line_info_table *table)
> else if (get_attr_min_length (insn) > 0)
> reset = 1;
>
> - if (reset > 0)
> + if (reset > 0 && !RESETTING_VIEW_P (table->view))
> RESET_NEXT_VIEW (table->view);
> }
>
> @@ -3235,9 +3237,10 @@ static GTY(()) bitmap zero_view_p;
> that must be view number zero. Otherwise, ZERO_VIEW_P is allocated
> and views label numbers recorded in it are the ones known to be
> zero. */
> -#define ZERO_VIEW_P(N) (zero_view_p \
> - ? bitmap_bit_p (zero_view_p, (N)) \
> - : (N) == 0)
> +#define ZERO_VIEW_P(N) ((N) == (var_loc_view)0 \
> + || (N) == (var_loc_view)-1 \
> + || (zero_view_p \
> + && bitmap_bit_p (zero_view_p, (N))))
>
> /* Return true iff we're to emit .loc directives for the assembler to
> generate line number sections.
> @@ -27210,6 +27213,18 @@ create_label:
> last_postcall_label = ggc_strdup (loclabel);
> }
> newloc->label = last_postcall_label;
> + /* ??? This view is at last_label, not last_label-1, but we
> + could only assume view at last_label-1 is zero if we could
> + assume calls always have length greater than one. This is
> + probably true in general, though there might be a rare
> + exception to this rule, e.g. if a call insn is optimized out
> + by target magic. Then, even the -1 in the label will be
> + wrong, which might invalidate the range. Anyway, using view,
> + though technically possibly incorrect, will work as far as
> + ranges go: since L-1 is in the middle of the call insn,
> + (L-1).0 and (L-1).V shouldn't make any difference, and having
> + the loclist entry refer to the .loc entry might be useful, so
> + leave it like this. */
> newloc->view = view;
> }
>
> @@ -27391,7 +27406,7 @@ new_line_info_table (void)
> table->file_num = 1;
> table->line_num = 1;
> table->is_stmt = DWARF_LINE_DEFAULT_IS_STMT_START;
> - RESET_NEXT_VIEW (table->view);
> + FORCE_RESET_NEXT_VIEW (table->view);
>
> return table;
> }
> @@ -27475,6 +27490,7 @@ dwarf2out_begin_function (tree fun)
> tail_call_site_count = 0;
>
> set_cur_line_info_table (sec);
> + FORCE_RESET_NEXT_VIEW (cur_line_info_table->view);
> }
>
> /* Helper function of dwarf2out_end_function, called only after emitting
> @@ -27572,10 +27588,44 @@ dwarf2out_source_line (unsigned int line, unsigned
> int column,
> {
> unsigned int file_num;
> dw_line_info_table *table;
> + static var_loc_view lvugid;
>
> - if (debug_info_level < DINFO_LEVEL_TERSE || line == 0)
> + if (debug_info_level < DINFO_LEVEL_TERSE)
> return;
>
> + table = cur_line_info_table;
> +
> + if (line == 0)
> + {
> + if (debug_variable_location_views
> + && output_asm_line_debug_info ()
> + && table && !RESETTING_VIEW_P (table->view))
> + {
> + /* If we're using the assembler to compute view numbers, we
> + can't issue a .loc directive for line zero, so we can't
> + get a view number at this point. We might attempt to
> + compute it from the previous view, but since we're
> + omitting the line number entry, we might as well omit the
> + view number as well. That means pretending it's a view
> + number zero, which might very well turn out to be
> + correct. */
> + if (!zero_view_p)
> + zero_view_p = BITMAP_GGC_ALLOC ();
> + bitmap_set_bit (zero_view_p, table->view);
> + if (flag_debug_asm)
> + {
> + char label[MAX_ARTIFICIAL_LABEL_BYTES];
> + ASM_GENERATE_INTERNAL_LABEL (label, "LVU", table->view);
> + fprintf (asm_out_file, "\t%s line 0, omitted view ",
> + ASM_COMMENT_START);
> + assemble_name (asm_out_file, label);
> + putc ('\n', asm_out_file);
> + }
> + table->view = ++lvugid;
> + }
> + return;
> + }
> +
> /* The discriminator column was added in dwarf4. Simplify the below
> by simply removing it if we're not supposed to output it. */
> if (dwarf_version < 4 && dwarf_strict)
> @@ -27584,7 +27634,6 @@ dwarf2out_source_line (unsigned int line, unsigned
> int column,
> if (!debug_column_info)
> column = 0;
>
> - table = cur_line_info_table;
> file_num = maybe_emit_file (lookup_filename (filename));
>
> /* ??? TODO: Elide duplicate line number entries. Traditionally,
> @@ -27646,13 +27695,6 @@ dwarf2out_source_line (unsigned int line, unsigned
> int column,
> }
> if (debug_variable_location_views)
> {
> - static var_loc_view lvugid;
> - if (!lvugid)
> - {
> - gcc_assert (!zero_view_p);
> - zero_view_p = BITMAP_GGC_ALLOC ();
> - bitmap_set_bit (zero_view_p, 0);
> - }
> if (!RESETTING_VIEW_P (table->view))
> {
> /* When we're using the assembler to compute view
> @@ -27673,7 +27715,7 @@ dwarf2out_source_line (unsigned int line, unsigned
> int column,
> }
> else
> {
> - if (!table->in_use)
> + if (FORCE_RESETTING_VIEW_P (table->view))
> fputs (" view -0", asm_out_file);
> else
> fputs (" view 0", asm_out_file);
> @@ -27684,6 +27726,8 @@ dwarf2out_source_line (unsigned int line, unsigned
> int column,
> known to be zero, because then we may be able to
> optimize out locviews that are all zeros, so take
> note of it in zero_view_p. */
> + if (!zero_view_p)
> + zero_view_p = BITMAP_GGC_ALLOC ();
> bitmap_set_bit (zero_view_p, lvugid);
> table->view = ++lvugid;
> }
> @@ -27696,17 +27740,22 @@ dwarf2out_source_line (unsigned int line, unsigned
> int column,
>
> targetm.asm_out.internal_label (asm_out_file, LINE_CODE_LABEL,
> label_num);
>
> - if (debug_variable_location_views && table->view)
> + if (debug_variable_location_views && !RESETTING_VIEW_P (table->view))
> push_dw_line_info_entry (table, LI_adv_address, label_num);
> else
> push_dw_line_info_entry (table, LI_set_address, label_num);
> if (debug_variable_location_views)
> {
> + bool resetting = FORCE_RESETTING_VIEW_P (table->view);
> + if (resetting)
> + table->view = 0;
> +
> if (flag_debug_asm)
> fprintf (asm_out_file, "\t%s view %s%d\n",
> ASM_COMMENT_START,
> - table->in_use ? "" : "-",
> + resetting ? "-" : "",
> table->view);
> +
> table->view++;
> }
> if (file_num != table->file_num)
> diff --git a/gcc/testsuite/gcc.dg/graphite/pr84404.c
> b/gcc/testsuite/gcc.dg/graphite/pr84404.c
> new file mode 100644
> index 000000000000..858e651cfab7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/graphite/pr84404.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -ftree-parallelize-loops=2 -floop-nest-optimize -g" } */
> +
> +int te[9];
> +
> +void
> +dt (int cz)
> +{
> + while (cz < 1)
> + {
> + int xy;
> +
> + for (xy = 0; xy < 9; ++xy)
> + te[xy] = 0;
> +
> + ++cz;
> + }
> +}
>
>
> --
> Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/ FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
