Re: unshare_all_rtl missed some rtl

[Jeffrey A Law <law at cygnus dot com>]

  In message <200001122044.MAA01531@localhost.cygnus.com>you write:
  > Anyway, as I said I have a patch to fix this, it's being tested now
  > but unfortunately it looks like it will fail (it is causing ~20 more
  > failures).  m68k testing on target boards is like racing snails (and
  > just when you think you're going to win, the silly things turn around
  > and start going backwards :-).
Sigh.   FYI -- there is a m68k unix box in Sunnyvale again.  It may or may
not be busy (ask Angela, it's one of her machines).  That would probably be
faster than downloading testcases to m68k target boards ;-)  Even better you
can do a bootstrap and run plumhall/perennial.

  > This is the code in copy_rtx_if_shared that does this unsharing.  This
  > is already executed on all insns, and I think all stack slot DECLs (my
  > patch adds function incoming argument DECLs).
  > 
  >     case MEM:
  >       /* A MEM is allowed to be shared if its address is constant.
  > 
  > 	 We used to allow sharing of MEMs which referenced 
  > 	 virtual_stack_vars_rtx or virtual_incoming_args_rtx, but
  > 	 that can lose.  instantiate_virtual_regs will not unshare
  > 	 the MEMs, and combine may change the structure of the address
  > 	 because it looks safe and profitable in one context, but
  > 	 in some other context it creates unrecognizable RTL.  */
  >       if (CONSTANT_ADDRESS_P (XEXP (x, 0)))
  > 	return x;
  > 
  >       break;
  > 
  > Since my patch does not affect how rtl is shared inside or between
  > insns, I'm confident it wouldn't break a hash table of the kind you
  > describe.  That doesn't mean that the hash table isn't _already_
  > broken.  You could change this code without needing to revert my
  > patch, although it might make my patch pointless.
OK.  Thanks for the explanation.


  > Isn't the sequence
  > 
  > rtl generation
  > procedure inlining (for inlining using RTL)
  > unshare_all_rtl
  > virtual register instantiation
  > optimization passes (starting with jump)
Good point.  So this can't possibly effect RTL inlining.  I still worry a
little about instantiation, but as I mentioned, I'm more than willing to
defer to Jim on this once since he understands this stuff better and has
been guiding you to a fix.

  > I noticed your cygnus-local hack involving unshare_all_rtl.  
  > For those on the list outside Red Hat, the hack is that
  > unshare_all_rtl is called at the end of purge_addressof, with this
  > comment:
  > 
  >   /* REGs are shared.  purge_addressof will destructively replace a REG
  >      with a MEM, which creates shared MEMs.
  > 
  >      Unfortunately, the children of put_reg_into_stack assume that MEMs
  >      referring to the same stack slot are shared (fixup_var_refs and
  >      the associated hash table code).
  > 
  >      So, we have to do another unsharing pass after we have flushed any
  >      REGs that had their address taken into the stack.
  > 
  >      It may be worth tracking whether or not we converted any REGs into
  >      MEMs to avoid this overhead when it is not needed.  */
  > 
  > >From the description it sounds like it is doing more unsharing, not
  > less.  The PA port has this in CONSTANT_ADDRESS_P:
  > 
  > #define CONSTANT_ADDRESS_P(X) \
  >   ((GET_CODE (X) == LABEL_REF || GET_CODE (X) == SYMBOL_REF		\
  >    || GET_CODE (X) == CONST_INT || GET_CODE (X) == CONST		\
  >    || GET_CODE (X) == HIGH) 					
  > 	\
  >    && (reload_in_progress || reload_completed || ! symbolic_expression_p (X
  > )))
  > 
  > So I take it the replacement in question looked something like
  > 
  > (const (addressof (reg something)))
  > 
  > and the CONST was shared???
No, there's no CONST at all.  There was a good discussion of this problem
on our internal list.  If anyone is interested I'll dig it out.  I don't
think either Jim or I were actually pleased with the second unshare_all_rtl
call, but I didn't have any luck dealing with the problem as we were
eliminating the ADDRESSOF expressions.

jeff