public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Sandiford <richard@codesourcery.com>
To: gcc-patches@gcc.gnu.org
Subject: Re: RFA: Stop df from generating SUBREG REG_NOTES
Date: Wed, 25 Jul 2007 22:50:00 -0000	[thread overview]
Message-ID: <87k5sodu51.fsf@firetop.home> (raw)
In-Reply-To: <874pk8eiea.fsf_-_@firetop.home> (Richard Sandiford's message of 	"Fri\, 13 Jul 2007 10\:56\:45 +0100")

Ping^2

Cross builds of mips-linux-gnu have been broken since the
dataflow merge, so it would be nice to get this sorted.

Richard

Richard Sandiford <rsandifo@nildram.co.uk> writes:
> Ping!
> Richard Sandiford <rsandifo@nildram.co.uk> writes:
>> Richard Sandiford <rsandifo@nildram.co.uk> writes:
>>> Jan Hubicka <jh@suse.cz> writes:
>>>> The users of df_set_note all just take the registers from dataflow
>>>> information and call the function to produce REG_DEAD/REG_UNUSED notes.
>>>> This all is going to introduce invalid sharing for subregs in all cases
>>>> and if we are going to produce the note with SUBREG, we simply must
>>>> produce the copy. (If we are very very cautious, we probably can have
>>>> some cache since we are probably recomputing those notes quite often,
>>>> but I doubt it is worth the extra complexity and we can see it easilly
>>>> from memory tester)
>>>>
>>>> Note that copy_rtx does nothing for REG, since registers are shared, so
>>>> it should not be overly expensive and we simply must produce the
>>>> duplicate for the case of SUBREG or any other unshared RTL expression.
>>>>
>>>> However I now wonder why do we want to see subregs there at all.
>>>> Originally I assumed that we now do partial liveness for multiple word
>>>> values, but it is not the case.  Perhaps the right thing to do is to
>>>> simply strip away the SUBREG and keep only the REG itself?
>>>> At least before dataflow, I don't think we produced SUBREGs in those
>>>> notes, I might be wrong.
>>>
>>> FWIW, I agree.  Several places in gcc seem to assume that REG_DEAD
>>> and REG_UNUSED notes are plain REGs, and access REGNO unconditionally.
>>> It even looks like df_ref_record was written with that in mind;
>>> there's an "oldreg" variable alongside "reg", presumably so that
>>> "reg" could be turned into "SUBREG_REG (oldreg)" if necessary,
>>> but the two variables stay the same throughout.
>>>
>>> This was causing a segmentation fault while building libjava
>>> for mipsel-linux-gnu.  We had a REG_DEAD (subreg:DI (reg:DF $f0))
>>> note, and some code was applying REGNO to that subreg.
>>>
>>> Also, there seems to be some redundancy in df_mw_hardreg.
>>> It has both a mw_reg field and a loc field, but it looks like
>>> all uses of loc can be replaced with uses of mw_reg.
>>>
>>> I'm testing the patch below.  I wouldn't normally post a patch
>>> before it has finished testing, but since the question is being
>>> actively debated, I thought I might as well.
>>
>> Of course, the problem with doing that is that I changed my mind
>> soon afterwards.  The patch survived testing, but as discussed
>> on #gcc yesterday, I no longer think using the inner REG is
>> unconditionally the right thing to do.
>>
>> The SUBREG handling in df_ref_record assumes that we can ask the port
>> for the range of registers that (subreg:M (reg:N R)) occupies, even when
>> mode M is not valid for hard register R.  This assumption was inherited
>> from flow, and I'm assuming that it is correct.  (The bug I'm fixing is
>> a malformed register note rather than inaccurate liveness information.)
>> The range of registers for M is obviously not always going to be the
>> same as the range for N, even though it was in the MIPS case.
>>
>> The code that adds notes will try to use the original mw_reg if
>> all of the referenced register is dead (for REG_DEAD) or unused
>> (for REG_UNUSED).  It will otherwise create notes for each individual
>> hard register.
>>
>> I think we should always take the latter approach -- creating individual
>> notes for each hard register -- when the mw_reg is a SUBREG rather than
>> a REG.  This ties in quite naturally with the DF_REF_PARTIAL setting;
>> as far as I can tell, we always consider a reference to a SUBREG of a
>> multiword hard register to be partial, even if the SUBREG appears to be
>> the same width as the inner register.  (And if we decide to drop the
>> DF_REF_PARTIAL in some cases, the approach taken in the patch below
>> should still work for the cases that remain DF_REF_PARTIAL.)
>>
>> Kenny mentioned the old flow code on #gcc yesterday.  I think flow
>> used to treat a _use_ of a SUBREG like a use of the whole SUBREG_REG.
>>> From mark_used_regs:
>>
>>     case SUBREG:
>> #ifdef CANNOT_CHANGE_MODE_CLASS
>>       if (flags & PROP_REG_INFO)
>> 	record_subregs_of_mode (x);
>> #endif
>>
>>       /* While we're here, optimize this case.  */
>>       x = SUBREG_REG (x);
>>       if (!REG_P (x))
>> 	goto retry;
>>       /* Fall through.  */
>>
>>     case REG:
>>       /* See a register other than being set => mark it as needed.  */
>>       mark_used_reg (pbi, x, cond, insn);
>>       return;
>>
>> df is now trying to be more accurate by tracking individual hard
>> registers in the SUBREG_REG.  The notes we add must clearly agree
>> with the main df liveness information, so I don't think the old
>> flow code sets any precedent here.  We're now tracking more
>> registers individually, so adding more individual notes seems
>> fair game.
>>
>> Flow used to track sets individually.  From mark_sets_1:
>>
>>     case SUBREG:
>>       if (REG_P (SUBREG_REG (reg)))
>> 	{
>> 	  enum machine_mode outer_mode = GET_MODE (reg);
>> 	  enum machine_mode inner_mode = GET_MODE (SUBREG_REG (reg));
>>
>> 	  /* Identify the range of registers affected.  This is moderately
>> 	     tricky for hard registers.  See alter_subreg.  */
>>
>> 	  regno_last = regno_first = REGNO (SUBREG_REG (reg));
>> 	  if (regno_first < FIRST_PSEUDO_REGISTER)
>> 	    {
>> 	      regno_first += subreg_regno_offset (regno_first, inner_mode,
>> 						  SUBREG_BYTE (reg),
>> 						  outer_mode);
>> 	      regno_last = (regno_first
>> 			    + hard_regno_nregs[regno_first][outer_mode] - 1);
>>
>> 	      /* Since we've just adjusted the register number ranges, make
>> 		 sure REG matches.  Otherwise some_was_live will be clear
>> 		 when it shouldn't have been, and we'll create incorrect
>> 		 REG_UNUSED notes.  */
>> 	      reg = gen_rtx_REG (outer_mode, regno_first);
>> 	    }
>>
>> If every register in [regno_first, regno_last] was unused, flow would
>> add a REG_UNUSED note for the REG created above.  If only some of those
>> registers were unused, flow would add notes for the individual unused
>> registers.  As I said on IRC yesterday, I'm not convinced the first
>> behaviour was a good idea.  That REG might well be invalid (otherwise
>> why have a SUBREG in the first place?) and it's better not to have
>> invalid registers floating around.
>>
>> Bootstrapped & regression-tested on x86_64-linux-gnu.  Also tested by
>> building mipsel-linux-gnu (which failed before the patch).  OK to install?
>>
>> Richard
>>
>>
>> gcc/
>> 	* df.h (df_mw_hardreg): Remove "loc" field.
>> 	* df-scan.c (df_ref_record): Don't set it.  Remove redundant
>> 	local variable.
>> 	* df-problems.c (df_whole_mw_reg_unused_p): New function,
>> 	split out from df_set_unused_notes_for_mw.  Return false for
>> 	partial references.  Assert that mw_reg is a REG when returning true.
>> 	(df_set_unused_notes_for_mw): Use it.  Use mw_reg instead of *loc.
>> 	(df_whole_mw_reg_dead_p): New function, split out from
>> 	df_set_dead_notes_for_mw.  Return false for partial references.
>> 	Assert that mw_reg is a REG when returning true.
>> 	(df_set_dead_notes_for_mw): Use it.  Use mw_reg instead of *loc.
>> 	Remove redundant bitmap check.
>>
>> Index: gcc/df.h
>> ===================================================================
>> --- gcc/df.h	(revision 126108)
>> +++ gcc/df.h	(working copy)
>> @@ -312,7 +312,6 @@ struct dataflow
>>  struct df_mw_hardreg
>>  {
>>    rtx mw_reg;                   /* The multiword hardreg.  */ 
>> -  rtx *loc;			/* The location of the reg.  */
>>    enum df_ref_type type;        /* Used to see if the ref is read or write.  */
>>    enum df_ref_flags flags;	/* Various flags.  */
>>    unsigned int start_regno;     /* First word of the multi word subreg.  */
>> Index: gcc/df-scan.c
>> ===================================================================
>> --- gcc/df-scan.c	(revision 126108)
>> +++ gcc/df-scan.c	(working copy)
>> @@ -2627,7 +2627,6 @@ df_ref_record (struct df_collection_rec 
>>  	       enum df_ref_type ref_type, 
>>  	       enum df_ref_flags ref_flags) 
>>  {
>> -  rtx oldreg = reg;
>>    unsigned int regno;
>>  
>>    gcc_assert (REG_P (reg) || GET_CODE (reg) == SUBREG);
>> @@ -2658,7 +2657,7 @@ df_ref_record (struct df_collection_rec 
>>  	{
>>  	  /* Sets to a subreg of a multiword register are partial. 
>>  	     Sets to a non-subreg of a multiword register are not.  */
>> -	  if (GET_CODE (oldreg) == SUBREG)
>> +	  if (GET_CODE (reg) == SUBREG)
>>  	    ref_flags |= DF_REF_PARTIAL;
>>  	  ref_flags |= DF_REF_MW_HARDREG;
>>  
>> @@ -2666,7 +2665,6 @@ df_ref_record (struct df_collection_rec 
>>  	  hardreg->type = ref_type;
>>  	  hardreg->flags = ref_flags;
>>  	  hardreg->mw_reg = reg;
>> -	  hardreg->loc = loc;
>>  	  hardreg->start_regno = regno;
>>  	  hardreg->end_regno = endregno - 1;
>>  	  hardreg->mw_order = df->ref_order++;
>> Index: gcc/df-problems.c
>> ===================================================================
>> --- gcc/df-problems.c	(revision 126109)
>> +++ gcc/df-problems.c	(working copy)
>> @@ -3717,6 +3717,32 @@ df_set_note (enum reg_note note_type, rt
>>    return old;
>>  }
>>  
>> +/* A subroutine of df_set_unused_notes_for_mw, with a selection of its
>> +   arguments.  Return true if the register value described by MWS's
>> +   mw_reg is known to be completely unused, and if mw_reg can therefore
>> +   be used in a REG_UNUSED note.  */
>> +
>> +static bool
>> +df_whole_mw_reg_unused_p (struct df_mw_hardreg *mws,
>> +			  bitmap live, bitmap artificial_uses)
>> +{
>> +  unsigned int r;
>> +
>> +  /* If MWS describes a partial reference, create REG_UNUSED notes for
>> +     individual hard registers.  */
>> +  if (mws->flags & DF_REF_PARTIAL)
>> +    return false;
>> +
>> +  /* Likewise if some part of the register is used.  */
>> +  for (r = mws->start_regno; r <= mws->end_regno; r++)
>> +    if (bitmap_bit_p (live, r)
>> +	|| bitmap_bit_p (artificial_uses, r))
>> +      return false;
>> +
>> +  gcc_assert (REG_P (mws->mw_reg));
>> +  return true;
>> +}
>> +
>>  /* Set the REG_UNUSED notes for the multiword hardreg defs in INSN
>>     based on the bits in LIVE.  Do not generate notes for registers in
>>     artificial uses.  DO_NOT_GEN is updated so that REG_DEAD notes are
>> @@ -3729,7 +3755,6 @@ df_set_unused_notes_for_mw (rtx insn, rt
>>  			    bitmap live, bitmap do_not_gen, 
>>  			    bitmap artificial_uses)
>>  {
>> -  bool all_dead = true;
>>    unsigned int r;
>>    
>>  #ifdef REG_DEAD_DEBUGGING
>> @@ -3737,18 +3762,11 @@ df_set_unused_notes_for_mw (rtx insn, rt
>>      fprintf (dump_file, "mw_set_unused looking at mws[%d..%d]\n", 
>>  	     mws->start_regno, mws->end_regno);
>>  #endif
>> -  for (r=mws->start_regno; r <= mws->end_regno; r++)
>> -    if ((bitmap_bit_p (live, r))
>> -	|| bitmap_bit_p (artificial_uses, r))
>> -      {
>> -	all_dead = false;
>> -	break;
>> -      }
>> -  
>> -  if (all_dead)
>> +
>> +  if (df_whole_mw_reg_unused_p (mws, live, artificial_uses))
>>      {
>>        unsigned int regno = mws->start_regno;
>> -      old = df_set_note (REG_UNUSED, insn, old, *(mws->loc));
>> +      old = df_set_note (REG_UNUSED, insn, old, mws->mw_reg);
>>  
>>  #ifdef REG_DEAD_DEBUGGING
>>        df_print_note ("adding 1: ", insn, REG_NOTES (insn));
>> @@ -3773,6 +3791,34 @@ df_set_unused_notes_for_mw (rtx insn, rt
>>  }
>>  
>>  
>> +/* A subroutine of df_set_dead_notes_for_mw, with a selection of its
>> +   arguments.  Return true if the register value described by MWS's
>> +   mw_reg is known to be completely dead, and if mw_reg can therefore
>> +   be used in a REG_DEAD note.  */
>> +
>> +static bool
>> +df_whole_mw_reg_dead_p (struct df_mw_hardreg *mws,
>> +			bitmap live, bitmap artificial_uses,
>> +			bitmap do_not_gen)
>> +{
>> +  unsigned int r;
>> +
>> +  /* If MWS describes a partial reference, create REG_DEAD notes for
>> +     individual hard registers.  */
>> +  if (mws->flags & DF_REF_PARTIAL)
>> +    return false;
>> +
>> +  /* Likewise if some part of the register is not dead.  */
>> +  for (r = mws->start_regno; r <= mws->end_regno; r++)
>> +    if (bitmap_bit_p (live, r)
>> +	|| bitmap_bit_p (artificial_uses, r)
>> +	|| bitmap_bit_p (do_not_gen, r))
>> +      return false;
>> +
>> +  gcc_assert (REG_P (mws->mw_reg));
>> +  return true;
>> +}
>> +
>>  /* Set the REG_DEAD notes for the multiword hardreg use in INSN based
>>     on the bits in LIVE.  DO_NOT_GEN is used to keep REG_DEAD notes
>>     from being set if the instruction both reads and writes the
>> @@ -3783,7 +3829,6 @@ df_set_dead_notes_for_mw (rtx insn, rtx 
>>  			  bitmap live, bitmap do_not_gen,
>>  			  bitmap artificial_uses)
>>  {
>> -  bool all_dead = true;
>>    unsigned int r;
>>    
>>  #ifdef REG_DEAD_DEBUGGING
>> @@ -3799,25 +3844,13 @@ df_set_dead_notes_for_mw (rtx insn, rtx 
>>      }
>>  #endif
>>  
>> -  for (r = mws->start_regno; r <= mws->end_regno; r++)
>> -    if ((bitmap_bit_p (live, r))
>> -	|| bitmap_bit_p (artificial_uses, r)
>> -	|| bitmap_bit_p (do_not_gen, r))
>> -      {
>> -	all_dead = false;
>> -	break;
>> -      }
>> -  
>> -  if (all_dead)
>> +  if (df_whole_mw_reg_dead_p (mws, live, artificial_uses, do_not_gen))
>>      {
>> -      if (!bitmap_bit_p (do_not_gen, mws->start_regno))
>> -	{
>> -	  /* Add a dead note for the entire multi word register.  */
>> -	  old = df_set_note (REG_DEAD, insn, old, *(mws->loc));
>> +      /* Add a dead note for the entire multi word register.  */
>> +      old = df_set_note (REG_DEAD, insn, old, mws->mw_reg);
>>  #ifdef REG_DEAD_DEBUGGING
>> -	  df_print_note ("adding 1: ", insn, REG_NOTES (insn));
>> +      df_print_note ("adding 1: ", insn, REG_NOTES (insn));
>>  #endif
>> -	}
>>      }
>>    else
>>      {

  reply	other threads:[~2007-07-25 21:57 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-06-26  3:43 RTL sharing tester (for testing) Jan Hubicka
2007-06-27 15:55 ` Kenneth Zadeck
2007-06-27 18:17 ` Diego Novillo
2007-06-27 21:43   ` Jan Hubicka
2007-06-28  2:02     ` Kaz Kojima
2007-06-28  4:24       ` Jan Hubicka
2007-06-28  8:11         ` Paolo Bonzini
2007-06-28 17:16         ` Kenneth Zadeck
2007-06-28 18:26           ` Jan Hubicka
2007-06-28 19:57             ` Kenneth Zadeck
2007-06-28 20:27             ` Richard Sandiford
2007-06-29  8:29               ` Richard Sandiford
2007-06-29 12:11                 ` Kenneth Zadeck
2007-06-29 13:35                   ` Paolo Bonzini
2007-06-29 13:37                 ` Bernd Schmidt
2007-06-29 13:39                   ` Jan Hubicka
2007-06-29 16:55                     ` Richard Sandiford
2007-07-13 10:15                 ` RFA: Stop df from generating SUBREG REG_NOTES Richard Sandiford
2007-07-25 22:50                   ` Richard Sandiford [this message]
2007-07-27  0:01                   ` Ian Lance Taylor
2007-06-28 20:19           ` RTL sharing tester (for testing) Paolo Bonzini
2007-06-27 23:06   ` Mark Mitchell
2007-06-27 23:06     ` Jan Hubicka
2007-06-27 23:12       ` Eric Christopher
2007-06-28  0:48         ` Jan Hubicka
2007-06-28 22:24           ` Eric Christopher
2007-06-29 23:14           ` Graham Stott
2007-06-30  0:17             ` Jan Hubicka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k5sodu51.fsf@firetop.home \
    --to=richard@codesourcery.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).