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: [PATCH] resubmitted for review to fix modulo scheduling. COMMITTED


Bernd,

This patch makes the way that you wish strict_lower_part subreg stores
the default and adds the flag DF_PARTIAL to allow the semantics that
appear to be useful under certain restricted conditions.   I understand
your view that the dataflow information that is produced with DF_PARTIAL
is not obvious, however I believe that I have shown that those semantics
can be useful for at least a few optimizations.  Since the model that df
supports is where one produces dataflow information only for the pass in
which it is used, it seems that there is little harm with allowing this
as an optional knob for the person writing the pass to turn. 

I have bootstrapped and regression tested this patch on the platforms:

x86_64-unknown-linux-gnu
powerpc64-unknown-linux-gnu
i686-pc-linux-gnu

Kenny

2006-03-28  Daniel Berlin  <dberlin@dberlin.org>
        Kenneth Zadeck <zadeck@naturalbridge.com>

        * df.h (DF_PARTIAL): New flag to control generation of
    DF_REF_PARTIAL.
    * df-scan.c (df_ref_record, df_def_record_1, df_uses_record)
    Control the generation of DF_REF_PARTIAL and phantom uses
    for strict_lower_part subregs.



Bernd Schmidt wrote:
> To summarize what we went through on IRC (correct me if I
> misunderstood anything): the current code is in fact a bit
> inconsistent; Kenny's arguments were based on the belief that we did
> not generate uses for partial defs, while in fact we do.  So, the DCE
> example that was posted would in fact not work currently.
>
> Kenny proposed a flag that could be passed to DF to switch between two
> possible behaviours of the DF code.  One would generate both a use and
> a set for a partial access, the DF_REF_PARTIAL bit would become
> superfluous.  The other would not create uses for partial defs, and
> instead mark them specially with DF_REF_PARTIAL, so that users of the
> df information would treat them differently.
>
> Kenneth Zadeck wrote:
>
>> It is not my contention that your way is wrong or inferior.  My
>> contention is that different optimizations will want to have this corner
>> of the problem resolved in different ways:
>
> I could be convinced with a really good example, but so far I'm of the
> opinion that partialness of a set is not a property that is
> interesting: neither for the DCE example, nor for webs.  In both cases
> it is a only a special case of something more general, and mixing up
> the separate concepts is not helpful, and rather a sign of unclear
> thinking.  To see this, always consider the equivalence of the
> following two instructions (slightly changed from last time):
>
>   (set (strict_low (op 0)) (op 1))
>   (set (op 0) (ior (and (match_dup 0) 0xFFFFFF00) (zero_extend (op 1)))
>
> and ask yourself why any pass would want to treat them differently. 
> To recap for DCE: the interesting property is that a use is only used
> to produce a dead def, and therefore doesn't count when determining
> the liveness of its own defs.  Using partialness here is a hack which
> would catch only a tiny fraction of the cases where this situation can
> be detected (how many strict_low_part sets in a parallel with an
> independent other set have you ever seen?).
>
>> As far as web analysis goes, the idea is to split a pseudo into two or
>> more new pseudo if the graphs can be separated.  If the only connection
>> between two components of a graph is thru an instruction that has a
>> subreg set then your way of representing it, as a reference and then a
>> full def would break the optimization. 
>
> If you rely solely on DF_REF_PARTIAL for this, then your optimization
> is too simpleminded - it's already broken since it doesn't take
> match_dup into account.  Again, partialness is not the property you're
> looking for: instead it's the question whether, if you have both a use
> and a def in an insn, you can have different rtl for the two.  A
> partial set is one case where you obviously can't because you have
> only one location for it, but a match_dup is another.  If you're
> looking at this optimization from the perspective of a register
> allocator, then you may also want to consider matching constraints.
>
> The webs that are produced without treating partial defs specially are
> perfectly correct according to the definition of a web.  You just
> can't necessarily use them directly for any given purpose - you may
> have to join multiple webs according to other criteria.  web.c already
> does things like that, e.g.
>   /* A READ_WRITE use requires the corresponding def to be in the same
>      register.  Find it and union.  */
> So, web.c already expects that the df code uses the normal, expected
> definition of defs and uses, and tries to deal with it where it must.
> (I'm not convinced that the bit in that function which allegdly
> handles match_dups actually works.  I don't think df-scan tries to
> look for match_dups and mark them as READ_WRITE refs - it would be
> real nifty if it did.)
>
>>> However, the result of such an exercise is not to proclaim the winner,
>>> it would be to fix the algs so that they are getting the best
>>> information and need the fewest workarounds.
>
> I'm not interesting in finding a winner.  I'm interested in fixing
> code which I believe does highly nonobvious things which are not
> documented and may therefore lead to really nasty surprises when
> anyone tries to use it.  I'll say it more plainly: what we have now is
> broken.
>
> The original topic was the use of DF_REF_PARTIAL in the RU and RD
> problems (possibly others, I haven't looked), where I still believe we
> should remove the special case.  To spell out my argument again in all
> detail:
> 1. These are supposed to be textbook algorithms (at least they use the
>    same names), so if they don't behave as you'd expect after reading a
>    compiler book, then either the algorithm, or its documentation is
>    seriously wrong.
> 2. One possible set of definitions is that a USE of a register is a
>    place where its value is used, and a DEF of a register is a place
>    where its value is modified.  I claim that this is a fairly good
>    definition which would surprise few people.  It's also demonstrably
>    the right definition to use for a fair number of optimization passes,
>    such as (for example) fwprop, which could break if the definition was
>    different.
> 3. If the df code wants to use a different definition where a partial
>    set is not a USE, even though it uses the previous value, or not a
>    DEF, even though it modifies the value, then that needs to be
>    documented very thoroughly.  There also needs to be a really good
>    reason for it.
> 4. If no reasonable example of the usefulness of such nonobvious
>    definitions can be found, then there is no reason for the compiler to
>    implement them at all.  Any such example would have to take into
>    account the example of the two same-semantics instructions I posted
>    above.
>
> The issue needs addressing, either with documentation, or by changing
> the code.  So far, I have seen no evidence of (4).  Hence, I must ask
> you to remove the DF_REF_PARTIAL handling from the places in
> df-problems.c where it would lead to unexpected behaviour.
>
>
> Bernd
>
> PS: Danny, your email is bouncing.

Index: df-scan.c
===================================================================
--- df-scan.c	(revision 114314)
+++ df-scan.c	(working copy)
@@ -1101,7 +1101,7 @@ df_ref_record (struct dataflow *dflow, r
 	  struct df_insn_info *insn_info = DF_INSN_GET (df, insn);
 	  /* 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 (oldreg) == SUBREG && dflow->flags & DF_PARTIAL)
 	    ref_flags |= DF_REF_PARTIAL;
 	  ref_flags |= DF_REF_MW_HARDREG;
 	  hardreg = pool_alloc (problem_data->mw_reg_pool);
@@ -1238,7 +1238,8 @@ df_def_record_1 (struct dataflow *dflow,
      they are wrapped in a strict lowpart, and not partial otherwise.
   */
   if (GET_CODE (dst) == SUBREG && REG_P (SUBREG_REG (dst))
-      && dst_in_strict_lowpart)
+      && dst_in_strict_lowpart
+      && dflow->flags & DF_PARTIAL)
     flags |= DF_REF_PARTIAL;
     
   if (REG_P (dst)
@@ -1320,7 +1321,8 @@ df_uses_record (struct dataflow *dflow, 
 
     case SUBREG:
       /* While we're here, optimize this case.  */
-      flags |= DF_REF_PARTIAL;
+      if (dflow->flags & DF_PARTIAL)
+	flags |= DF_REF_PARTIAL;
       /* In case the SUBREG is not of a REG, do not optimize.  */
       if (!REG_P (SUBREG_REG (x)))
 	{
@@ -1343,7 +1345,8 @@ df_uses_record (struct dataflow *dflow, 
 	switch (GET_CODE (dst))
 	  {
 	    case SUBREG:
-	      if (df_read_modify_subreg_p (dst))
+	      if (!(dflow->flags & DF_PARTIAL)
+		  && df_read_modify_subreg_p (dst))
 		{
 		  df_uses_record (dflow, &SUBREG_REG (dst), 
 				  DF_REF_REG_USE, bb,
Index: df.h
===================================================================
--- df.h	(revision 114314)
+++ df.h	(working copy)
@@ -220,6 +220,9 @@ struct dataflow
 #define DF_HARD_REGS	     1	/* Mark hard registers.  */
 #define DF_EQUIV_NOTES	     2	/* Mark uses present in EQUIV/EQUAL notes.  */
 #define DF_SUBREGS	     4	/* Return subregs rather than the inner reg.  */
+#define DF_PARTIAL           8  /* Record the DF_REF_PARTIAL flag on subreg */
+                                /* accesses and do not generate a use for */
+                                /* read-modify-write subregs.  */
   /* Flags that control the building of chains.  */
 #define DF_DU_CHAIN          1    /* Build DU chains.  */  
 #define DF_UD_CHAIN          2    /* Build UD chains.  */

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