1st giv recombination optimization patch installment updated

Jeffrey A Law law@cygnus.com
Tue Aug 31 22:41:00 GMT 1999


  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



More information about the Gcc-patches mailing list