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]

Re: Reload patch v4



At a high level -- excellent work.  If every patch of this size was
as well written my life would be a hell of a lot easier. 


On a general note, I'm happy to see that you broke up some of those
huge functions into more bite-sized pieces.  It really does make the
code easier to read and understand at a high level.  It makes the
patchfile really big, but that's OK.


stupid.c:

  You added an include for insn-config.h, but did not
  add it to the dependency list for stupid.o.

  You need a comment before find_clobbered_regs

  Can find_clobbered_regs be called with SUBREG (hard reg)?  If it
  does, then you need to handle it.

  In stupid_life_analysis, the new arrays should be allocated with
  xmalloc and free'd later.  If you look at the current sources you'll
  find the other arrays are allocated/freed in that manner.

  Do we still need reg_where_born?  Any other arrays that need to be
  eliminated?


caller-save.c:

  In insert_save_restore, don't you need to set the mode and code for
  insns created by this routine?  The old code used to do this, but
  I do not see that your code does this.  Actually I can see that
  you *might* not need to set the mode right now, but I'm not 100%
  sure.


reorg.c:

  Instead of #if 0-ing the code which assumed spill regs were dead
  at the end of the block, just delete it.  I also believe you can
  eliminate the code to keep track of max_label_num_before_reload,
  but you should check and make sure the only uses are in the
  reorg.c code we are deleting.


local-alloc.c:

  I understand your desire to remove the scratch handling in local
  alloc.  But I'm not sure it's wise at this point.

  In general, the more things we can allocate in local alloc the
  better -- our algorithms for local register are much better than
  those for global register allocation.

  In theory, we should be able to revisit this after dealing with
  the rest of your reload changes, right?

  Also note that whether or not we keep the local-alloc.c code
  determines what we do with related hunks of code in global.c
  and reload1.c.

  


global.c:

  In reg_becomes_live I see a "Subreg here" warning.  I do think
  that warning can trigger.

  In build_insn_chain, don't you have to be prepared to handle a
  REG_DEAD note for a SUBREG?  Or is there some reason this can
  not happen?


reload.h:

  Why is struct insn_chain's definition dependent on SET_HARD_REG_BIT?


reload1.c (the biggie):

     available hard regs can be found.  Spilling can invalidate more
     insns, requiring additional need for reloads, so we must keep checking
     until the process stabilizes.
+    If we have register life information, it is often possible to avoid
+    spilling all the pseudos allocated to a reload register.
I would actually reword this comment.  With your patch we should have
register life information most of the time right?  If so, then we should
really reword that entire paragraph to indicate that the normal behavior
is a local spill and that we do a global spill only if we do not have
register lifetime available.

In the comment before pseudo_forbidden_regs, you mention "additionally
to those in forbidden_regs".  However, forbidden_regs as far as I can
tell has been totally eliminated.  I think this comment needs updating.

Aren't fixed registers and eliminable registers still in "bad_spill_regs"?
The current comment does not list them explicitly (I think it should 
mention them explicitly).

I think you should delete all the code related to "used_spill_regs"
since we're deleting the only user of this information in reorg.c


What's the difference between reload_firstobj and reload_startobj?


You should allocate reg_old_renumber and pseudo_forbidden_regs with
xmalloc and release their memory explicitly (in general, we want to
move away from using alloca and we're starting with things which can
potentially be large allocations).

Can you explain the problems with caller-save and the new reload code
in a little more detail?


+   /* Let's try without this, I can't see a reason for doing this.  */
+ #if defined ELIMINABLE_REGS && 0
+   /* If registers other than the frame pointer are eliminable, mark them as
+      poor choices.  */
+   for (i = 0; i < NUM_ELIMINABLE_REGS; i++)
+     SET_HARD_REG_BIT (bad_spill_regs, reg_eliminate[i].from);
+
+ #endif

You should reenable this code.  Imagine a machine which uses an arg
pointer ;-)  You really don't want to spill it if at all possible.
Similarly if the machine has other eliminable regs.


I think we need to fix forget_old_reloads_1 to handle cases where we
store into a pseudo which happens to be allocated to a hard reg that
has also been used as a spill reg.  Imagine

	Use reg 0 as an output reload for some value V

	Psuedo X is made live and is allocated to reg 0
	[ ... ]
	Pseudo X dies

	Use of V which needs an input reload.

In this case we might try and use reg0 to satisfy the input reload
even though it no longer holds the value we need.

You need comments before find_tworeg_group and find_group.

In all it looks pretty good.   If you could make the requested changes
and send a fresh diff it would be appreciated.  I'll probably start some
tests for targets I work on with the next version of these patches.


jeff

  

  



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