Fix postreload-gcse treatment of call-clobbered registers

Eric Botcazou ebotcazou@libertysurf.fr
Thu May 24 15:16:00 GMT 2007


> As far as I can tell, this makes reg_set_between_after_reload_p
> identical to reg_set_between_p; the innards of both are equivalent to
> reg_set_p.  And that's probably as it ought to be.  reg_set_between_p
> should certainly provide accurate information for hard registers, and
> nothing in the current reg_set_between_after_reload_p is really
> optimised for non-pseudo usage.

Even better!

> Just so we're on the same page, I'm not sure what you mean by the
>
> "same change", or by your earlier comment:
> > Finally, record_opr_changes should also be modified along the lines of
> > your change.
>
> record_opr_changes already processes every hard register in
> regs_invalidated_by_call, so I think that part is OK.

Ah, yes, I was confused.

> I've implemented the find_reg_fusage change in the patch below.

Thanks.

> I think we should also check the reg_avail_info entry for every
> individual register, rather than just the first.  I've added a
> new function (reg_changed_after_insn_p) to do that.

I think you're right.

> @@ -495,12 +509,12 @@ oprs_unchanged_p (rtx x, rtx insn, bool
>        if (after_insn)
>  	/* If the last CUID setting the insn is less than the CUID of
>  	   INSN, then reg X is not changed in or after INSN.  */
> -	return reg_avail_info[REGNO (x)] < INSN_CUID (insn);
> +	return !reg_changed_after_insn_p (x, INSN_CUID (insn) - 1);
>        else
>  	/* Reg X is not set before INSN in the current basic block if
>  	   we have not yet recorded the CUID of an insn that touches
>  	   the reg.  */
> -	return reg_avail_info[REGNO (x)] == 0;
> +	return !reg_changed_after_insn_p (x, 0);

I think that both comments are now outdated and should be removed, the logic 
is now fully encapsulated in reg_changed_after_insn_p.

>  static bool
>  reg_set_or_used_since_bb_start (rtx reg, basic_block bb, rtx up_to_insn)
>  {
> -  rtx insn, start = PREV_INSN (BB_HEAD (bb));
> -
> -  if (reg_avail_info[REGNO (reg)] != 0)
> -    return true;
> -
> -  insn = reg_used_between_after_reload_p (reg, start, up_to_insn);
> -  if (! insn)
> -    insn = reg_set_between_after_reload_p (reg, start, up_to_insn);
> -
> -  if (insn)
> -    reg_avail_info[REGNO (reg)] = INSN_CUID (insn);
> -
> -  return insn != NULL_RTX;
> +  return (reg_changed_after_insn_p (reg, 0)
> +	  || reg_used_between_p (reg, PREV_INSN (BB_HEAD (bb)), up_to_insn));

I think that reg_set_or_used_since_bb_start now serves no useful purpose and 
should be inlined back in its caller.

> 	* postreload-gcse.c (reg_changed_after_insn_p): New function.
> 	(oprs_unchanged_p): Use it to check all registers in a REG.
> 	(record_opr_changes): Look for clobbers in CALL_INSN_FUNCTION_USAGE.
> 	(reg_set_between_after_reload_p): Delete.
> 	(reg_used_between_after_reload_p): Likewise.
> 	(reg_set_or_used_since_bb_start): Use reg_changed_after_insn_p
> 	to check reg_avail_info for every individual hard register.
> 	Use reg_used_between_p instead of reg_used_between_after_reload_p.
> 	Do not call reg_set_between_after_reload_p.
> 	(eliminate_partially_redundant_load): Use reg_set_between_p
> 	instead of reg_set_between_after_reload_p.
> 	* rtlanal.c (reg_set_p): Check whether REG overlaps
> 	regs_invalidated_by_call, rather than just checking the
> 	membership of REGNO (REG).

OK if you remove the couple of comments and optionally inline r_g_o_u_s_b_s, 
in which case no need to retest either, I've bootstrapped/regtested this 
version with RTL checking for C/C++/Ada on x86, the important hunk being

@@ -1037,7 +972,8 @@ eliminate_partially_redundant_load (basi
 
   /* Check that the loaded register is not used, set, or killed from the
      beginning of the block.  */
-  if (reg_set_or_used_since_bb_start (dest, bb, insn))
+  if (reg_changed_after_insn_p (dest, 0)
+      || reg_used_between_p (dest, PREV_INSN (BB_HEAD (bb)), insn))
     return;

Thanks for your cooperation. ;-)

-- 
Eric Botcazou



More information about the Gcc-patches mailing list