On Aug 21, 2017, Richard Biener <richard.guent...@gmail.com> wrote:
> On Fri, Aug 18, 2017 at 11:20 PM, Alexandre Oliva <aol...@redhat.com> wrote:
>> Besides implementing these new features, the patch contains multiple
>> fixes for -fcompare-debug errors detected at various optimization
>> levels, arising mainly from the introduction of begin stmt and inlined
>> entry point markers.
> Can you try to split those out?
I'm pretty sure I split out before the ones that were not triggered by
the introduction of the new debug position markers. The remaining ones
thus don't necessarily stand on their own, since the conditions needed
to trigger them are not necessarily exercised by the compiler. Plus,
most of the fixes introduce references to the new classification of
debug stmts and insns.
Would it be sensible and acceptable to bring the bits that introduce the
new classification along with their new uses, rather than make two
revisions at the same spots?
> +gno-statement-frontiers
> +Common Driver RejectNegative Var(debug_nonbind_markers_p, 0) Init(2)
> +Don't enforce progressive recommended breakpoint locations.
> +
> +gstatement-frontiers
> +Common Driver RejectNegative Var(debug_nonbind_markers_p, 1)
> +Emit progressive recommended breakpoint locations.
> others get away with a single flag and Init(-1). That is,
> -gstatement-frontiers
> should set it to 1 already and -gno- to 0. Why do you need the explicit
> entry for gno-..?
Good question. I vaguely recall wondering about that myself when
copying from some other option. Will investigate.
> for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
> - if (DEBUG_INSN_P (insn))
> + if (BIND_DEBUG_INSN_P (insn))
> {
> DEBUG_BIND_INSN_P? GIMPLE has gimple_debug_bind_p ...
Yeah, gimple notation seems to go from most general to most specific
(first gimple, then debug, then bind), whereas we go from specific to
general in the rtl macro names (insn at the end, debug just before it,
and now bind just before it). I suppose debug bind insn would work, as
well, though it feels as awkward and inconsistent as e.g. Brazil, São
Paulo, Earth to me. Or, for that matter, Aug 22, 2017, ;-) so I guess
one could get used to it ;-)
> + /* If the function has too many markers, drop them while expanding. */
> + if (cfun->debug_marker_count
> + >= PARAM_VALUE (PARAM_MAX_DEBUG_MARKER_COUNT))
> + cfun->debug_nonbind_markers = false;
> +
> if they are not a problem up until here why care now?
IIRC we do have a limit for VTA notes too, but there's a C++ testcase
(g++.dg/tree-ssa/pr14703.C) that expands and inlines fibonacci template
functions so deep, more than doubling the number of statements at all
but the base recursion levels, so we'd end up with over 2^{85+} debug
stmts if we didn't cut them off somehow.
> That said,
> what's the number
> of markers per GIMPLE stmt for optimized builds of tramp3d-v4.cpp? [it has
> around 10 original function calls per generated assembler line]
I'm not sure how to count gimple stmts precisely, but I can count DEBUG
ones exactly. Compiling tramp3d-v4 with -O2 -g and either -g0, -g, or
-gno-statement-frontiers, respectively, the 228t.optimized dump contains:
$ grep -c '^ *# DEBUG ' tramp3d-v4.ii.228t.optimized-*
tramp3d-v4.ii.228t.optimized-g0:0
tramp3d-v4.ii.228t.optimized-markers:982493
tramp3d-v4.ii.228t.optimized-noSFN:743940
$ wc -l tramp3d-v4.ii.228t.optimized-*
238038 tramp3d-v4.ii.228t.optimized-g0
1220531 tramp3d-v4.ii.228t.optimized-markers
981978 tramp3d-v4.ii.228t.optimized-noSFN
So it seems like SFN and IEPM add a little over one extra debug stmt per
real stmt, whereas VTA added 3. For this testcase, anyway.
That's not surprising. Most useful statements have at least one
(variable-binding) side effect, and quite often more than one, so VTA
notes tend to dominate the begin_stmt markers.
Even inlined calls, that might have a different ratio, it's still one
inlined entry point and possibly one begin stmt marker, vs one bind stmt
per parameter, plus the inlined code.
> Would a better option be to condense multiple adjacent notes to a single one?
> That way we'd have a natural bound as fallback.
That wouldn't help with the fibonacci testcase, I'm afraid, but it's an
interesting idea I had not explored.
I guess it wouldn't help much in typical code, since typical code has
side effects and thus binding stmts, so we wouldn't find long streams of
nonbinding markers. Even the atypical case of the fibonacci template
function set would end up being cut-off once I get to adding bindings
for return values, which is in my list of things to explore.
> I expect heavily abstracted C++ to blow up GIMPLE IL considerably that way...
> Did you see what these do to memory/compile-time use with a LTO bootstrap?
I haven't observed any notable changes. I'll be glad to collect and
supply any specific measurements you choose.
> + if (MARKER_DEBUG_INSN_P (insn))
> + return true;
> +
> DEBUG_MARKER_INSN_P
See above.
> +/* Return the next insn after INSN that is not a DEBUG_INSN. This
> + routine does not look inside SEQUENCEs. */
> rtx_insn *
> -next_nonnote_insn_bb (rtx_insn *insn)
> +next_nondebug_insn (rtx_insn *insn)
> {
> while (insn)
> {
> sometimes I hate unified diffs .... this and the part following is
> unreadable.
*nod*; I recall having gone carefully over this part while preparing the
ChangeLog. Here it is, much easier to read: :-)
* emit-rtl.c (next_nondebug_insn, prev_nondebug_insn): Reorder.
(next_nonnote_nondebug_insn, prev_nonnote_nondebug_insn): Reorder.
(next_nonnote_nondebug_insn_bb): New.
(prev_nonnote_nondebug_insn_bb): New.
(prev_nonnote_insn_bb, next_nonnote_insn_bb): Remove.
> @@ -573,6 +573,8 @@ gsi_remove (gimple_stmt_iterator *i, bool
> remove_permanently)
> if (remove_permanently)
> {
> + if (gimple_debug_nonbind_marker_p (stmt))
> + cfun->debug_marker_count--;
> require_eh_edge_purge = remove_stmt_from_eh_lp (stmt);
> gimple_remove_stmt_histograms (cfun, stmt);
> }
> hmm, you're now relying on remove_permanently to tell the truth.
We don't really need to it to be exact. A rough estimate will do.
> + gdebug *p
> + = as_a <gdebug *> (
> + gimple_build_with_ops_stat (GIMPLE_DEBUG,
> + (unsigned)GIMPLE_DEBUG_BEGIN_STMT, 0
> + PASS_MEM_STAT));
> heh, we need a gimple_build <gdebug> (....) abstraction to make all this
> nicer (well, probably overkill, just used in internal gimple.c)
*nod* (as in, yeah, would be nice, but likely overkill, and more so
considering the reorg suggested below)
> Reading the patch both BEGIN_STMT and INLINE_ENTRY are just
> free-floating notes in GIMPLE to be not re-ordered. This means
> they could use gimple as base instead of gimple_statement_with_ops, no?
Interesting thought. Yeah, I guess that could be done. I'll give it a shot.
> Saving two pointers or nearly half size? Could it also hold a vector of
> locations so we can optimize adjacent stmt-start stmts,
As mentioned earlier in this message, I don't see that this occurs often
enough to make sense. In tramp3d-v4.ii.228t.optimized-markers, there
are only 2165 sequences with 2+ begin stmt markers one right after the
other; 92 with 3+, and only 2 with 4.
> maybe even also cover the inline thing?
Considering that the inlined entry point marker is most likely just
before the begin stmt marker of the first stmt of a function, it would
seem to very well make sense to combine them, or just set a flag in the
stmt to indicate what it is.
However, their locations are different, and they should ideally remain
so. I don't think growing the far more frequent begin stmt marker to
make room for an additional location for the infrequent inlined entry
point marker would make sense in general.
> +static location_t
> +expr_location (tree expr, location_t or_else = UNKNOWN_LOCATION)
> +{
> ...
> +static inline bool
> +expr_has_location (tree expr)
> ...
> _please_ do not just use lower-case names for sth subtly different
> from their upper-case part...
'k
> It would be nice to split out some of the mechanical changes, like
> function renaming or gsi_last_bb to gsi_last_nondebug_bb for example
> to shrink the parts that need "real" review. I'll happily ack those split
> out parts quickly.
> + else if (gimple_debug_nonbind_marker_p (stmt))
> + {
> + new_stmt = as_a <gdebug *> (gimple_copy (stmt));
> + }
> extra braces
Thanks, will fix.
> Skimmed over the whole patch now, I think it looks reasonably ok.
> Let's get rid of the noise and acks from the DWARF people.
*nod*
> Btw, just asking as I helped to get the GIMPLE FE in, did you
> consider adding GIMPLE FE support for the various debug stmts
> we then have? First thing would be arriving at a syntax I guess.
> __DEBUG x = ...; for binds, __STMT; __INLINE; for the other two?
> Not sure how to express they encode some location though...
> (binds have no location, right?)
I confess I hadn't considered any of that. I'll give it some thought.
Binds don't refer to a (program) location, yeah, whereas that's all the
new markers do. Inline markers actually reference the lexical block
containing the entire inlined copy of the function, and there's code
that depends on it, so we'd have to figure out some way to express that
sort of thing, in case the GIMPLE FE can't. Once we have that, the
syntax for debug stmts is just a breeze.
Thanks,
--
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
