On Tue, Aug 22, 2017 at 10:43 PM, Alexandre Oliva <aol...@redhat.com> wrote:
> 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?
Just separating the boilerplate changes out from the "meat" of the change
into a separate patch for easier reviewing would be nice.
>> +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.
Yeah, but I meant we've kept them throughout GIMPLE (for all functions!)
but are dropping them here at RTL expansion (which we'll have only a
single live RTL function at a time). That looks odd ;) You're already
dropping them at inlining as well so the RTL expansion check should
be superfluous IMHO (yeah, unrolling might push it over the edge for example
but all real issues should come from inlining).
>> 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.
Ok, thanks for the number ;)
> 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.
Hmm, yeah. I guess we'd have to have a multi-DEBUG_STMT that covers
not only multiple markers but also multiple binds. High GIMPLE has
nested stmts so it might be tempting to wrap adjacent debug-stmts into
a single one (basically make the IL walking overhead with debug stmts smaller).
Costs extra memory instead of less when compared to my idea of course.
>
>> 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.
Just curious if, for example, --with-build-config=bootstrap-lto
--enable-languages=c
--disable-multilib and make -j1 shows any difference in time and/or peak memory
use (the interesting peak memory use is that of the WPA phase).
>
>> + if (MARKER_DEBUG_INSN_P (insn))
>> + return true;
>> +
>
>> DEBUG_MARKER_INSN_P
>
> See above.
Yeah, please change those.
>
>> +/* 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.
;) Btw, this looked like part of the boilerplate that could be split out and
committed separately even? That is, the effective renaming this does
and changing users?
>
>> @@ -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.
Ok.
>
>> + 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.
Thanks. Will result in a somewhat awkward class hierarchy as those
can't be gdebug * then, but well ...
>> 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.
Ok, thanks for checking.
>
>> 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.
Agreed.
>> +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.
Ah, lexical blocks. Yeah, we'd have to add a syntactic way to
name them and refer to them. At least BINDS look easily doable ;)
For blocks there's the additional issue that the GIMPLE FE doesn't
really have them as GIMPLE doesn't really care unless you start
introducing locations and debug info ;) So with the GIMPLE FE
there's just the functions outermost scope/BLOCK. It shouldn't be
too hard to add though if we think it's useful.
Richard.
>
> 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
