This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: update address taken: don't drop clobbers


On Thu, 16 Oct 2014, Richard Biener wrote:

On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
On 10/15/14 08:35, Marc Glisse wrote:


Would that extra pass be acceptable?

Ugh, rather not.  We have too many passes ;)

I expected you would say that...

Otherwise, what do you think should be responsible for cleaning up the
dead assignments?


Does anyone have an opinion on which side needs to be improved? As a
reminder:

- we have a va_list with its address taken by va_start/va_end.
- fab lowers va_start/va_end and the list doesn't have its address taken
anymore.
- update_address_taken replaces the clobber: list =v {}; with an
assignment of an undefined value: list_6 = list_2(D);
- uninit warns about this.

Some possible directions:
- "prematurely" optimize in update_address_taken so we don't generate
the useless assignment.
- add a dce pass before uninit.

I tend to land on the side of minimizing false positives, so the comment
about PR18501 is a "don't care" to me.  If the optimizers remove a dead
assignment and we no longer warn about a potential uninitialized use in the
dead assignment, then I consider that good.  Not everyone agrees with that
way of thinking, obviously.

I agree with that.

So my inclination would be to evaluate independent of the pr18501 issues.
ie, what's the compile-time cost vs runtime benefit of running DCE here.
I'm guessing there's little runtime benefit for this particular case.

So my next line of thinking would be can we arrange to conditionally run
DCE?  ie, have update_address_taken signal that it did something that has a
reasonable chance of exposing dead code and only run DCE in that case.
Obviously this only helps if it rarely signals :-)  I don't think we have
any infrastructure for this right now.

Finally I'd look at how difficult it would be to have update_address_taken
cleanup after itself.   If the LHS is in SSA form, then if we find it has no
uses, can we just remove the assignment completely?

It doesn't even know that it has no uses (the variable still needs to be
written into SSA form).  OTOH it is a missed DSE opportunity before
update-address-taken?

Uh, we are talking about what happens to a clobber when the variable stops having its address taken. We don't want to DSE clobbers (do we?).

(and va_start is just an example, many transformations could cause this)

As of premature optimization - into-SSA could notice it created SSA
names with no uses and trigger a fast DCE.

I am looking into that, it must be doable. It seems not too hard, in maybe_register_def, to push all results from make_ssa_name to some data-structure (I don't think new_ssa_names gives me that list, but there may be other ways to get it without introducing yet another list), and either mark them as used in maybe_replace_use or get_reaching_def, or better loop through them at the end, checking has_zero_uses (it is a bit wasteful, only those coming from clobbers may have 0 uses (or we missed a dce/dse earlier), but it should be fast enough, even walking on all ssa_names should be fast enough). If we go to that much trouble, we may as well clean them while we are there. It isn't obvious to me how to notice unused new ssa_names more easily, to trigger a DCE.

Btw, I wonder what this odd folding of variadic builtins is about, and why
it is not done in the stdarg pass (and only there), which would be earlier.

pass_fold_builtins::execute has:

                case BUILT_IN_VA_START:
                case BUILT_IN_VA_END:
                case BUILT_IN_VA_COPY:
                  /* These shouldn't be folded before pass_stdarg.  */
                  result = optimize_stdarg_builtin (stmt);

while pass_stdarg only mentions them as:

          /* Don't look at __builtin_va_{start,end}, they are ok.  */

It does seem late, but that will be for someone else to look at.


By the way, I noticed something strange in tree-into-ssa.c:

static inline bool
is_old_name (tree name)
{
  unsigned ver = SSA_NAME_VERSION (name);
  if (!new_ssa_names)
    return false;
  return (ver < SBITMAP_SIZE (new_ssa_names)
          && bitmap_bit_p (old_ssa_names, ver));
}

There are a lot of "new" in this function about "old".

--
Marc Glisse


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]