[vta, vta4.4] merged with trunk and 4.4 @149247, updated VTA patchset

[Alexandre Oliva]

On Jul  7, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Tue, Jul 7, 2009 at 11:10 AM, Alexandre Oliva<aoliva@redhat.com> wrote:
>> Yeah, and that was done.  Should I have also removed the macros and
>> replaced the uses?  I can do that.

> Yes please.

Done, with additional cleanups.   Patch to follow.

>>> +/* Nonzero if is_gimple_debug() may possibly hold.  */
>>> +#define MAY_HAVE_DEBUG_STMTS    (flag_var_tracking_assignments)
>> 
>>> You have my maintainers vote to unconditionally enable
>>> flag_var_tracking_assignments.
>> 
>> Erhm...  Do you really want to *prevent* people from testing that VTA
>> isn't causing codegen changes?  That's what removing this macro and code
>> that tests it would accomplish.
>> 
>> I thought you were only defending it to be enabled by default.  I'm fine
>> with that, although I find it quite wasteful.  Even though the overhead
>> of carrying and updating the notes is low, it's not zero.

> Well, my main concern here is that if you can disable then people
> can argue generating different code is a bug.

I believe it should be regarded as a bug.  Different code means debug
info is getting in the way of optimizations.  It shouldn't.

> And I absolutely do believe your compare-debug coverage is going to
> prove itself not enough (consider all the passes that are not enabled
> by default).

If you mean using -fcompare-debug during bootstrap and testsuite, yes, I
agree.  But here's hope that at least some of the users who embrace VTA
will help us pinpoint differences, sacrificing some compile time
(-fcompare-debug).

> Thus, I'd rather trade optimization regressions in obscure cases
> for code generation differences -g vs. -g0 in obscure cases.

We don't have to pick one or the other.

The workflow I have in mind is that, if some obscure error comes up in
which recompiling with -g removes the error, -fcompare-debug can be used
to confirm and fix this compiler bug, and then the obscure error will be
debuggable with the aid of good debug information.

Of course, if people are cutting corners, they can always compile with
-g0 -fvar-tracking-assignments to work around any optimization
differences.


Given the existing, readily available workarounds, and the potential for
robustness testing that it brings, I'd much rather keep the options
available.

As for its default, I'd rather have VTA disabled when not emitting debug
info, to ease the minds of people who care about compile-time resource
utilization and not at all about debug information, but I wouldn't mind
if the community preferred to have VTA notes enabled by default.

> track it for what?  so maybe track_var_in_debug_stmt_p ()?

Investigation of how libjava deals with debug information led me to come
up with a function whose purpose is not only to check whether we should
track a variable, but also to tell what user variable it refers to.  In
Java, that uses DECL_HAS_VALUE_EXPR_P to map slots to user variables,
this makes all of the difference.

So now it is:

/* Given a tree for an expression for which we might want to emit
   locations or values in debug information (generally a variable, but
   we might deal with other kinds of trees in the future), return the
   tree that should be used as the variable of a DEBUG_BIND STMT or
   VAR_LOCATION INSN or NOTE.  Return NULL if VAR is not to be tracked.  */

tree
target_for_debug_bind (tree var)
{
  if (!MAY_HAVE_DEBUG_STMTS)
    return NULL_TREE;

  if (TREE_CODE (var) != VAR_DECL
      && TREE_CODE (var) != PARM_DECL)
    return NULL_TREE;

  if (DECL_HAS_VALUE_EXPR_P (var))
    return target_for_debug_bind (DECL_VALUE_EXPR (var));

#if 0
  /* Should we deal with DECL_DEBUG_EXPR_IS_FROM as well?  */
  if (DECL_DEBUG_EXPR_IS_FROM (var))
    return target_for_debug_bind (DECL_DEBUG_EXPR (var));
#endif

  if (DECL_IGNORED_P (var))
    return NULL_TREE;

  if (!is_gimple_reg (var))
    return NULL_TREE;

  return var;
}

> I am concerned that the prominent place of the function suggests
> it applies everywhere.  So yes, a gcc_assert on dominator
> availability would be ok.

Done

> I'm not sure.  I would have tried to minimize use of the function more
> (to the extent of removing it - heh).  What bad will happen if
> you just always punt if the BBs are the same?  I suspect most
> code-motion optimizations move/insert across BB boundaries
> anyway...

I'll try to assess the impact.

>> Maybe tree-ssa.c?

> Works for me as well.  Both are sort-of kitchen-sinks for all kind
> of stuff already ;)

Done

>> If tobb is NULL, we're removing the stmt.  Consider:
>> 
>>  x_N = whatever;
>>  ...
>>  # debug x => x_N
>> 
>> and then we delete the assignment to x_N.  What we want to do is to set
>> value to whatever x_N used to be assigned to, and substitute that, so
>> that we end up with:
>> 
>>  # debug x => whatever

> I had the impression that this is catched in release_ssa_name?

Yes.  Calling the adjust function.

>> Likewise, if we moved the assignment past the debug stmt, or to some
>> non-dominating location, we'd want to make this substitution, because
>> referring to x_N wouldn't be valid SSA any more, and so it would be
>> dropped afterwards.

> The substitution to what?  The original rhs?

Yup.  We want to replace the SSA_NAME that's in the debug stmt's RHS,
whose (SSA_NAME's) DEF we're removing, with the DEF's RHS.

> But that should be already in the debug-stmt as you carefully
> constructed them that way during into-ssa?

No.  Given:

  x_N = whatever;

we emit:

  # debug x => x_N

rather than

  # debug x => whatever

This should make for better location information when the variable
actually remains there.  It's better to provide a modifiable location
than a value that can only be inspected.

That said, we don't have machinery ATM to tell one from the other yet,
i.e., to tell whether, given x => x_N, we should emit a modifiable or
value-only location.  This is an improvement that's in my personal
roadmap, and I have a few heuristics that can be used for this purpose.

>> No, changing the DEF shouldn't call this adjust function.  This function
>> is to be called when a DEF is moved or removed.

> Hm, ok.  Can I have two wrappers around the main worker function
> named accordingly?  Thanks.

Err...  I'm sure you can, but I don't quite understand what you're
getting at.  How would you like these wrappers to be named?

> Ok, so I retract the lazy updating theory (I thought it would get rid
> of the move/remove functions).  Doing lazy updating during checking
> may introduce code generation differences with checking enabled/disabled
> (you shouldn't be changing IL during checking really).

That makes sense to me.  So I guess verify could ignore invalid SSA in
debug stmts, and expand could check that SSA references are ok before
expanding the stmt.  Makes sense?

>> It may have to be done elsewhere as well.

> in release_ssa_name only

No, it's three different problems that need to be addressed:

1. what happens when an SSA DEF is removed that was referenced in debug
stmts?

release_ssa_name calls adjust_... to propagate the RHS's DEF into
the debug uses

2. what happens when an SSA DEF is moved across a debug stmt that
references it?

whoever moved it should call adjust_..., for the same purpose, or...

3. what happens when we find an SSA use that is not dominated by the SSA
DEF?

In normal code, this is a bug caught by verify.

In debug stmts, this could also be regarded as a bug, and caught by
verify.  Then passes that make transformations that might result in this
kind of condition would be required to adjust debug stmts, in addition
to regular stmts that they must already adjust.

For now, a lazy solution was implemented, that involves testing at a
later time whether the DEF no longer dominates the debug USE, killing
the value in the debug stmt.  This is what the check_... function
implements.

-- 
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 Brazil Compiler Engineer