Errr, GCSE being overly conservative?

[Daniel Berlin <dan at cgsoftware dot com>]

Two things:
First, if I set MAX_PASSES up to 3, i don't get much of a significant
time increase (since it only bothers doing anything if it helped the
last pass anyway), however, I do get somewhat better code. Didn't use
to happen
I'm guessing that now that our global const/copy prop has improved, it
might be worth reinvestigating whether to up MAX_PASSES.

Remember, it's not going to do passes for fun anyway. It only does
another pass if the last one had some effect.

Second:

I know we claim it doesn't handle hard regs properly, but we have some
interesting conflicts of code in this regard:

In cprop_insn:

 else if (GET_CODE (src) == REG
      		      && REGNO (src) >= FIRST_PSEUDO_REGISTER
	       && REGNO (src) != regno)
	{

In record_set_info:
*************** record_set_info (dest, setter, data)
*** 1292,1298 ****
  {
    rtx record_set_insn = (rtx) data;
  
!   if (GET_CODE (dest) == REG && REGNO (dest) >= FIRST_PSEUDO_REGISTER)
      record_one_set (REGNO (dest), record_set_insn);
  }
  
--- 1290,1296 ----
  {
    rtx record_set_insn = (rtx) data;
  
!   if (GET_CODE (dest) == REG)
      record_one_set (REGNO (dest), record_set_insn);
  }
  

In hash_scan_set:

*************** hash_scan_set (pat, insn, set_p)
*** 2202,2208 ****
  
        /* Only record sets of pseudo-regs in the hash table.  */
        if (! set_p
- 	  && regno >= FIRST_PSEUDO_REGISTER
  	  /* Don't GCSE something if we can't do a reg/reg copy.  */
  	  && can_copy_p [GET_MODE (dest)]
  	  /* Is SET_SRC something we want to gcse?  */
--- 2199,2204 ----
*************** hash_scan_set (pat, insn, set_p)
*** 2230,2238 ****
  
        /* Record sets for constant/copy propagation.  */
        else if (set_p
- 	       && regno >= FIRST_PSEUDO_REGISTER
  	       && ((GET_CODE (src) == REG
- 		    && REGNO (src) >= FIRST_PSEUDO_REGISTER
  		    && can_copy_p [GET_MODE (dest)]
  		    && REGNO (src) != regno)
  		   || GET_CODE (src) == CONST_INT



These are there because we don't handlel hard regs properly, right?

However, we already seem to do the right thing in all of the
functions, for hard regs, that these functions call/use info from.


For instance, in compute_hash_table we have:

      for (insn = BLOCK_HEAD (bb);
	   insn && insn != NEXT_INSN (BLOCK_END (bb));
	   insn = NEXT_INSN (insn))
	{
#ifdef NON_SAVING_SETJMP 
	  if (NON_SAVING_SETJMP && GET_CODE (insn) == NOTE
	      && NOTE_LINE_NUMBER (insn) == NOTE_INSN_SETJMP)
	    {
	      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
		record_last_reg_set_info (insn, regno);
	      continue;
	    }
#endif
<...>
(still part of the same for loop)
  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
		if ((call_used_regs[regno]
		     && regno != STACK_POINTER_REGNUM
#if HARD_FRAME_POINTER_REGNUM != FRAME_POINTER_REGNUM
		     && regno != HARD_FRAME_POINTER_REGNUM
#endif
#if ARG_POINTER_REGNUM != FRAME_POINTER_REGNUM
		     && ! (regno == ARG_POINTER_REGNUM && fixed_regs[regno])
#endif
#if !defined (PIC_OFFSET_TABLE_REG_CALL_CLOBBERED)
		     && ! (regno == PIC_OFFSET_TABLE_REGNUM && flag_pic)
#endif

		     && regno != FRAME_POINTER_REGNUM)
		    || global_regs[regno])
		  record_last_reg_set_info (insn, regno);

	      if (! CONST_CALL_P (insn))
		record_last_mem_set_info (insn);

etc

We properly handle inserting as the last insn dealing with parameters
loads if the last_insn is a call_insn.

In fact, it seems the only reason we seem not to handle hard regs
properly, albeit in a conservative manner (which may be the best we can do
right now), is because of the  changes at the top.

If I perform the diffs i've listed at the top, i don't get more
failures, but i do get better code.
Of course, the cprop_insn one causes us to start propagating hard
regs, which may not be a good idea (then again, maybe it is. It's one
less register to allocate for in some cases).  However, it knows not
to propagate them across calls.

With the changes from the diff's above, for instance, PRE starts
eliminating those fun redundant stack calculations when it can, for example:

before:

<PRE does nothing>
(insn 63 92 65 (set (reg/f:SI 97)
        (plus:SI (reg/f:SI 31 r31)
            (const_int 16 [0x10]))) 36 {*addsi3_internal1} (nil)
    (expr_list:REG_EQUAL (plus:SI (reg/f:SI 31 r31)
            (const_int 16 [0x10]))
        (nil)))
now:

PRE: redundant insn 63 (expression 2) in bb 5, reaching reg is 101
PRE: bb 0, insn 101, copy expression 2 in insn 39 to reg 101

(expression 2 is (plus:SI (reg/f:SI 31 r31) (const_int 16 [0x10]))
...

(insn 63 95 65 (set (reg/f:SI 98)
        (reg:SI 101)) 286 {*movsi_internal1} (nil)
    (expr_list:REG_EQUAL (plus:SI (reg/f:SI 31 r31)
            (const_int 16 [0x10]))
        (nil)) 52 ("991016-1.c"))


In addition, things like:
Pattern (  0): (mem/f:SI (plus:SI (reg/f:SI 31 r31)
        (const_int 16 [0x10])) 5)
	 Loads : (insn_list 59 (nil))
	Stores : (nil)
At least get *considered* for load and store motion.
The same code that prevents it from doing things across calls it
shouldn't, prevents load/store motion across calls it shouldn't.

Why? Because it's copied code.
:)

So maybe somebody could point out, besides the fact that the above
code disables it, what it is exactly about hard regs we aren't
handling properly in GCSE?  Not what problems they raise, I mean i'm
looking for the reason the code at the beginning exists.   I don't
feel we are handling them as well as we could, but i think we *are*
being way too pessimistic.  We just don't seem to consider them at
all, when all the code besides deciding whether to consider them at
all, seems to have the necessary stuff in place to handle them in a
conservative enough manner as to not cause problems.
Preferrably, if someone can show me a test case that caused us to
decide that  the hard regs should not be considered at all, i'm
curious to see it.  Cause all  the test cases related to gcse or that
might show GCSE problems handling hard regs I can find, 
like 991016-1.c, work fine, and get better code. In fact, I
pulled the above examples from a compile of 991016-1.c. 
Maybe it's just my platform? (ppc linux)
If there is no such test case, I submit we should do something like
what i've done above.
If there is a test case, it should be in the testsuite, and it's
purpose, noted at the top of the file, so that the next person to go
spleunking around will get more failures when they try something like
I did, and go "Oh, that's why we can't do that".

--Dan

--
"The other night I came home late, and tried to unlock my house
with my car keys.  I started the house up.  So, I drove it
around for a while.  I was speeding, and a cop pulled me over.
He asked where I lived.  I said, "right here, officer".  Later,
I parked it on the freeway, got out, and yelled at all the cars,
"Get out of my driveway!"
"-Steven Wright