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]
Other format: [Raw text]

Re: [RS6000] Stack tie fix.


Hi Olivier,

On Thu, Apr 05, 2012 at 12:36:01PM +0200, Olivier Hainque wrote:
> On Apr 4, 2012, at 03:22 , Alan Modra wrote:
> > I'll see whether my approach fixes pr30282 for gcc-4.4 which has known
> > deficiencies in alias analysis.  Olivier, would you be kind enough to
> > backport and test against other versions of gcc that needed your
> > bigger hammer?

Well it turns out that gcc-4.4 still gets this testcase from pr30282
wrong.

int find_num(int i)
{
    int arr[5] = {0, 1, 2, 3, 4};
    return arr[i];
}

The read from arr[i] is scheduled past the frame teardown.

[snip]
>  There are lots of places in the emit_epilogue code that assign
>  frame_reg_rtx with different possible values, (hard_fp, sp, r11)

r12 too

>  before we first get to points where we emit ties. There are also
>  multiple places where we do emit ties.
> 
>  It is not at all obvious to me that the all places where we emit ties
>  have an accurate perception of what frame_reg's were possibly used before.
> 
>  IOW, I am not 100% convinced that we cannot have a bad case where
>  we emit a tie that misses a reg reference. The various paths are all
>  controlled by intricate conditions, so getting that 100% conviction
>  (at least on paper) isn't easy to me.
> 
>  Is it clearer to you ?

I spent quite some time looking at it. ;)  Have you spotted an error
somewhere, apart from spe_save_area_ptr not being mentioned in the
stack tie?  I left that one for a later fix since the SPE reg saves
use alias set 0 mems (which is another bug).

Hmm, I see I accidentally left out an assert from the stack tie
patch.  This one may make you feel a little better.  :)

  /* Update stack and set back pointer unless this is V.4,
     for which it was done previously.  */
  if (!WORLD_SAVE_P (info) && info->push_p
      && !(DEFAULT_ABI == ABI_V4 || crtl->calls_eh_return))
    {
      rtx copy_reg = NULL;

      /* If using some other frame reg previously, then we ought to
	 ensure it is mentioned in the stack tie emitted below.  */
      gcc_checking_assert (REGNO (frame_reg_rtx) == 1
			   || REGNO (frame_reg_rtx) == 12);


>  FWIW, I had made an experiment at trying to extract subfunctions,
>  which might help such reasoning.  Set of patches attached. This doesn't
>  apply to the current mainline, would need to refined, and we probably
>  couldn't/shouldn't put something like this on the path to the resolution
>  of our current issue, so this is JIC you are curious.

I think this is worthwhile doing, but more important to try to make
the logic simpler.  For example, this

  /* If we need to save CR, put it into r12 or r11.  */
  if (!WORLD_SAVE_P (info) && info->cr_save_p && frame_reg_rtx != frame_ptr_rtx)
    {
      rtx set;

      cr_save_rtx
	= gen_rtx_REG (SImode, DEFAULT_ABI == ABI_AIX && !saving_GPRs_inline
		       ? 11 : 12);

is better written as

  /* If we need to save CR, put it into r12 or r11.  */
  cr_save_regno = DEFAULT_ABI == ABI_AIX && !saving_GPRs_inline ? 11 : 12;
  if (!WORLD_SAVE_P (info)
      && info->cr_save_p
      && REGNO (frame_reg_rtx) != cr_save_regno)
    {
      rtx set;

      cr_save_rtx = gen_rtx_REG (SImode, cr_save_regno);

This way it's obvious why there is a test of frame_reg_rtx, and that
the test is correct.  ie. you aren't clobbering frame_reg_rtx with cr.

-- 
Alan Modra
Australia Development Lab, IBM


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