Re: Ada bootstrap failure due to new DCE bug

[Richard Henderson <rth at redhat dot com>]

On Mon, Aug 02, 2004 at 11:12:13PM -0400, Richard Kenner wrote:
> This popped up sometime in the last few days.
> 
> When compiling Increment_Serial_Number in ada/lib.adb, we drop an
> assignment:
...
> -   *tsn_7 = T.722_11;

Diego, something's horked here wrt memory tags.

What's happening is that *tsn_7 is being assigned TMT.1234, but
that memory tag isn't being considered needs_to_live_in_memory,
which leads need_to_preserve_store to the belief that it's ok to
delete the store.

It seems that needs_to_live_in_memory is returning false because 
DECL_NEEDS_TO_LIVE_IN_MEMORY_INTERNAL is never set for the memory tag.  

But of course it isn't that simple.  I tried setting this bit in
create_memory_tag.  But that only lasted until the next may_alias
pass, at which point we clear the bit in init_alias_info and never
set it again.

Which got me looking at what this bit is used for.  Or rather, not
used for.  It seems to me that we're not doing anything coherent
with this bit.  I can't remember what problem you were solving when
you introduced this bit, so I'm not sure what to look for.

It seems to me that there are four cases that we care about:

  (1) The variable is pristine.

  (2) The variable is variably indexed, but not addressable.  In this
      case it *does* need to live in memory, since there's basically
      no other way that variable indexing can be done without, um,
      interesting hardware.  But the variable is not aliased in any
      way, and all references to it will be through its decl.

  (3) The variable has its address taken, but the address is only used
      in controled ways, e.g. memset (&x, 0, sizeof(x)).  We don't 
      actually recognize or do anything with this case at present, but
      we should in the not-too-distant future, so I want you thinking
      about it from a representational standpoint.

      I envision the question I want to ask is "is this variable not
      aliased".  That is, it has to live in memory (as the program
      exists at the time of the question), but it's never accessed in
      any way that doesn't involve its decl, which means that a
      transformation may be able to look at the set of accesses and
      convert the variable to class 1 or 2.

  (4) The variable has its address taken, is a local automatic variable, 
      and the address does not escape.  In this case the variable is not
      always referenced via its own decl, which means it has aliases.
      It is not, however, call clobbered.

  (5) The variable's address escapes.  The variable must live in memory,
      is call clobbered, and all associated evil.

Are there any cases I missed?

Anyway, the patch below seems to work for the given test case.  Its
effect is to prevent anything in classes 3, 4, or 5 from being deleted
incorrectly.  It does, however, miss easy cases in classes 3 and 4
where we could delete dead code.

Note that class 2 is completely mis-handled at present.  I believe we
will incorrectly delete code in that case.

A conservative short-term fix would be to set TREE_ADDRESSABLE when
we find variable addressing.  We could also set TREE_STATIC on memory
tags in class 5, which would give us class groups of {1}, {2,3,4} and
{5}, which would give us about optimial results for DCE and shouldn't
be too horrible for everyone else.

For SRA we really want to have classes {1,2,3} distinguished from {4}.
I'm not sure how to do that, exactly.

Thoughts?


r~



PS: The test case doesn't fail in C because the C front end decomposes
&arr[n] to pointer arithmetic, and that formation somehow does the right
thing in tree-ssa-alias.c.  Not sure what the difference is, exactly.
I've had a patch pending for ages to get C and C++ to not decompose
array_refs so early, but it doesn't work 100%.  This may be the reason.


Index: tree-flow-inline.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-flow-inline.h,v
retrieving revision 2.18
diff -c -p -d -r2.18 tree-flow-inline.h
*** tree-flow-inline.h	23 Jul 2004 22:37:23 -0000	2.18
--- tree-flow-inline.h	5 Aug 2004 04:43:02 -0000
*************** static inline void
*** 648,655 ****
  mark_call_clobbered (tree var)
  {
    var_ann_t ann = var_ann (var);
-   /* Call-clobbered variables need to live in memory.  */
-   DECL_NEEDS_TO_LIVE_IN_MEMORY_INTERNAL (var) = 1;
    bitmap_set_bit (call_clobbered_vars, ann->uid);
  }
  
--- 648,653 ----
*************** static inline void
*** 658,664 ****
  mark_non_addressable (tree var)
  {
    bitmap_clear_bit (call_clobbered_vars, var_ann (var)->uid);
-   DECL_NEEDS_TO_LIVE_IN_MEMORY_INTERNAL (var) = 0;
    TREE_ADDRESSABLE (var) = 0;
  }
  
--- 656,661 ----
Index: tree-ssa-alias.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-ssa-alias.c,v
retrieving revision 2.22
diff -c -p -d -r2.22 tree-ssa-alias.c
*** tree-ssa-alias.c	30 Jul 2004 19:40:30 -0000	2.22
--- tree-ssa-alias.c	5 Aug 2004 04:43:02 -0000
*************** init_alias_info (void)
*** 390,396 ****
        EXECUTE_IF_SET_IN_BITMAP (call_clobbered_vars, 0, i,
  	{
  	  tree var = referenced_var (i);
- 	  DECL_NEEDS_TO_LIVE_IN_MEMORY_INTERNAL (var) = 0;
  
  	  /* Variables that are intrinsically call-clobbered (globals,
  	     local statics, etc) will not be marked by the aliasing
--- 390,395 ----
Index: tree.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree.c,v
retrieving revision 1.406
diff -c -p -d -r1.406 tree.c
*** tree.c	28 Jul 2004 23:44:48 -0000	1.406
--- tree.c	5 Aug 2004 04:43:03 -0000
*************** in_array_bounds_p (tree ref)
*** 5554,5560 ****
  bool
  needs_to_live_in_memory (tree t)
  {
!   return (DECL_NEEDS_TO_LIVE_IN_MEMORY_INTERNAL (t)
  	  || TREE_STATIC (t)
            || DECL_EXTERNAL (t)
  	  || (TREE_CODE (t) == RESULT_DECL
--- 5554,5560 ----
  bool
  needs_to_live_in_memory (tree t)
  {
!   return (TREE_ADDRESSABLE (t)
  	  || TREE_STATIC (t)
            || DECL_EXTERNAL (t)
  	  || (TREE_CODE (t) == RESULT_DECL
Index: tree.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree.h,v
retrieving revision 1.580
diff -c -p -d -r1.580 tree.h
*** tree.h	4 Aug 2004 20:37:38 -0000	1.580
--- tree.h	5 Aug 2004 04:43:03 -0000
*************** struct tree_binfo GTY (())
*** 2154,2168 ****
    (! DECL_CONTEXT (EXP)						\
     || TREE_CODE (DECL_CONTEXT (EXP)) == TRANSLATION_UNIT_DECL)
  
- /* Nonzero for a decl that has been marked as needing a memory slot.
-    NOTE: Never use this macro directly.  It will give you incomplete
-    information. Most of the time this bit will only be set after alias
-    analysis in the tree optimizers.  It's always better to call
-    needs_to_live_in_memory instead.  To mark memory variables use
-    mark_call_clobbered.  */
- #define DECL_NEEDS_TO_LIVE_IN_MEMORY_INTERNAL(DECL)		\
-   DECL_CHECK (DECL)->decl.needs_to_live_in_memory
- 
  /* Nonzero for a decl that cgraph has decided should be inlined into
     at least one call site.  It is not meaningful to look at this
     directly; always use cgraph_function_possibly_inlined_p.  */
--- 2154,2159 ----
*************** struct tree_decl GTY(())
*** 2235,2243 ****
    unsigned lang_flag_6 : 1;
    unsigned lang_flag_7 : 1;
  
-   unsigned needs_to_live_in_memory : 1;
    unsigned possibly_inlined : 1;
!   /* 14 unused bits.  */
  
    union tree_decl_u1 {
      /* In a FUNCTION_DECL for which DECL_BUILT_IN holds, this is
--- 2226,2233 ----
    unsigned lang_flag_6 : 1;
    unsigned lang_flag_7 : 1;
  
    unsigned possibly_inlined : 1;
!   /* 15 unused bits.  */
  
    union tree_decl_u1 {
      /* In a FUNCTION_DECL for which DECL_BUILT_IN holds, this is