Re: [PATCH] add 'rs6000_in_function_epilogue_p()' (Revised)

[Kevin Buettner <kevinb@redhat.com>]

On Mon, 5 Dec 2005 11:00:51 -0800
Paul Gilliam <pgilliam@us.ibm.com> wrote:

> I made the changes to address Mark's concerns.

You missed adding spaces around at least one of the operators.  The one
that I noticed was the `-' in:

    +   for (scan pc = pc-PPC INSN SIZE;

Also, regarding the following bit:

    +  int opcode  = (insn>>26) & 0x03f;
    +  int sd      = (insn>>21) & 0x01f;
    +  int a       = (insn>>16) & 0x01f;
    +  /*  b       = (insn>>11) & 0x01f  */
    +  int subcode = (insn>> 1) & 0x3ff;
    +  /*  rc      =  insn      & 0x001  */

I like the use of indentation to line things up, but our coding
standard is to remove extraneous spaces.  (Because that's what GNU
indent would do.  Yeah, I agree it's lame...)

Ah, I just noticed that you missed the spaces around the >> operators too.

> But because of the interest this has drawn, I thought I better ask
> again before I commited the patch.

Thanks for asking again.  It was indeed useful to reread the other
comments.

Could I ask you to make the following modifications to your patch?

1) Abort a forward scan (and return 0) if any control transfer instruction
   other than blr is found.

2) Abort a backward scan (and return 0) if any control transfer instruction
   is found.

3) Place a hard limit on the number of instructions scanned both
   forwards and backwards.  Epilogues are typically not very big and
   scanning to the ends of a large function starting at the middle seems
   rather wasteful.

Kevin