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

Kevin Buettner
2005-12-07T20:58:20Z

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