Re: 1st giv recombination optimization patch installment updated

[Jeffrey A Law <law@cygnus.com>]

  In message <199908192048.VAA01987@phal.cygnus.co.uk>you write:
  > I've just finished updating and re-testing my first optimization installmen
  > t
  > patch.  As mentioned in my previous posting, it:
  > - replaces find_life_end with find_giv_uses
  > - allows keeping better track of actual biv / giv lifetimes, thus needing
  >   less registers when givs are set long before they are used
  > - allows to ignore unused givs
  > 
  > Overall, this should reduce register pressure from strength-reduced loops,
  > thus giving smaller and faster code.  We can still do a lot better, but
  > that is the subject of other patches - which depend on this one.
  > 
  > Wed May 12 21:49:04 1999  J"orn Rennecke <amylaar@cygnus.co.uk>
  > 
  >         * loop.h (struct induction): New member leading_combined.
  >         * loop.c (note_giv_use): New argument from_combined.
  >         Changed all callers.
  >         Update start_luid and leading_combined.
  >         (strength_reduce, record_giv): Clear leading_combined field.
  >         (recombine_givs): Remove bogus index / giv lockstep looping.
  >         Use leading_combined to determine if giv must be moved.
  > 
  > Wed May  5 21:15:40 1999  J"orn Rennecke <amylaar@cygnus.co.uk>
  > 
  >         *loop.c (recombine_givs): Set ix field after sorting.
  > 
  >         (recombine_givs): Allocate new_reg for derived givs.
  >         (strength_reduce): Simplify code dealing with derived givs knowing
  >         that the new_reg is allocated early now.
  > 
  >         * loop.h (struct induction): New member live_after_loop.
  >         * loop.c (find_life_end): Deleted.
  >         (find_giv_uses, note_giv_use, cmp_giv_by_value_and_insn): New funct
  > ions.
  >         (strength_reduce): Set new field in struct induction for givs.
  >         (recombine_givs): New parameters.  Changed caller.
  >         (record_giv): Set new field in struct induction.
  >         (recombine_givs): Givs life only from their first use to their
  >         last use if there is no intervening label; we then move the
  >         set just before the use if the giv gets derived.
Thanks.  A few questions & comments.

First, from a high level, what was wrong with find_end_life?  Yes, I've heard
it was fragile.  Maybe what I'm looking for is why was find_end_life fragile
and what makes this scheme more robust?



  > ! /* The last label we encountered while scanning forward for giv uses.
  > !    Is initialized to SCAN_START (not necessarily a label) in recombine_gi
  > vs.  */
  > ! static rtx loop_last_label;
I wonder if we can come up with a better name for this variable.  It seems
kind of hokey to assign a non-label to "loop_last_label".


  > !       if (loop_insn_first_p (stats[v->ix].start_insn, loop_last_label)
  > ! 	  && (loop_insn_first_p (loop_last_label, insn)
  > ! 	      || loop_insn_first_p (insn, stats[v->ix].start_insn)))
Can you explain via a short comment what this conditional is testing?  I
think I know, but it would be a lot more obvious if you stated in plain
english what you were testing so that people not as familiar with the code
could understand its purpose a little easier.


  > ! 
  > !       /* Update start_luid now so that we won't loose this information it
  > ! 	 when we invalidate start_insn.  */
  > !       for (p = insn; INSN_UID (p) >= max_uid_for_loop; )
  > ! 	p = PREV_INSN (p);
That is a fairly odd looking loop.  Is there some reason not to write it like
this (ie like we do most other loops)?

  for (p = insn; INSN_UID (p) >= max_uid_for_loop; p = PREV_INSN (p))
    ;



Or 

   rtx p = insn

   [ ... ]
   while (INSN_UID (p) >= max_uid_for_loop)
      p = PREV_INSN (p);


Either seems better than what you wrote.




  >   	      {
  > ! 		/* Since we are setting a non-ignored general induction
  > ! 		   variable, this insn will be changed or go away, hence
  > ! 		   we don't have to consider uses in the SET_SRC.  */
  > ! 		return;
  > ! 	      }
Can you explained why we do not have to consider this case in more detail?
I just don't follow why you can ignore this case.  Similarly for a reduced
DEST_ADDR giv.

  > !       if ((uid_luid[REGNO_FIRST_UID (REGNO (v->dest_reg))]
  > ! 	   > INSN_LUID (loop_start))
  > ! 	  && (uid_luid[REGNO_LAST_UID (REGNO (v->dest_reg))]
  > ! 	      < INSN_LUID (loop_end)))
Should those be >= and <=?  


  > ! 		      /* We don't have recorded which givs are life after the
  > ! 			 loop only because their giv register is life, or
  > ! 			 (also) because a combined giv is life after the loop,
  > ! 			 so just pretend it is the latter if any other givs
  > ! 			 have been combined with this one.  */
"We don't have recorded"?  Did you mean "We have not recorded" or 
"We do not have to record"?


I'd like to take another look at the patch once you've updated it.  I think
we're probably on the track track, but I'll need some of the clarifications
mentioned above to really understand what you're trying to do.

Thanks,
jeff