Re: PATCH for sibcalls on i386

[Fergus Henderson <fjh at cs dot mu dot OZ dot AU>]

Hi Andreas,

That looks like a good start, but there are still a few major issues
that need to be addressed before this patch could be acceptable.

On 23-Sep-2002, Andreas Bauer <baueran@in.tum.de> wrote:
> Attached is a patch to gcc-3.2 that adds a few new call patterns to the
> i386 machine description which allow sibcall optimisation for indirect
> calls.

It's probably best to develop such patches against the
gcc-3_4-basic-improvements branch.

> I'm overriding the "fndecl == NULL_TREE" check in "calls.c" and cover it
> in FUNCTION_OK_FOR_SIBCALL instead.  The macro will decide whether an
> indirect call is a candidate for sibcall optimisation and if so, the
> changed machine descriptions may be put into place at the end of
> compilation.  Non-i386 architectures, however, still check for fndecl in
> "calls.c", because they do not yet allow any indirect calls at all!

Putting a check for x86 in calls.c is messy -- it breaks the idea that
target-specific stuff should go in target-specific files.

It would be much cleaner to do as Richard Henderson suggested,
and check for fndecl in FUNCTION_OK_FOR_SIBCALL for all
targets, not just x86.  For the non-x86 targets, just
have FUNCTION_OK_FOR_SIBCALL return false if fndecl is null.

> So, the patch doesn't break anthing, as far as I know.

I'm afraid the patch does break things.
It breaks cross-compilation, and introduces unwanted warnings.

> I'm aware that the patch is too small to actually make a huge difference
> on gcc as a whole, but if the concept proves useful, then it could be
> applied to other architectures as well and we could remove the test
> "fndecl == NULL_TREE" totally from the file "calls.c".

You're being a bit too tentative here.
It would be better to go ahead and do this step now.

> +       /* Additional call patterns in i386.md allow i386 to do
> + 	 optimized indirect calls.  Other platforms may/should
> + 	 follow this example.
> + 	 In fact, it is sufficient on i386 to merely test with the
> + 	 FUNCTION_OK_FOR_SIBCALL macro, as it contains certain
> + 	 conditionals for handling indirect calls.  */
> + 
> + #ifndef __i386
>         || fndecl == NULL_TREE
> + #endif

This test for __i386 is wrong.  Whether or not __i386 is defined here
will depend on whether the host system is an x86, but whether or not
indirect calls matter depends on whether the *target* system is an x86.
So this will break cross-compilation.

The phrase ``elegance is not optional'' comes to mind ;-)
Doing it cleanly, in the way Richard Henderson suggested,
would avoid this bug.

> + int
> + i386_function_ok_for_sibcall (decl, exp)
> +      tree decl, exp;
> + {
...
> +   /* Function call can not be optimized.  */
> +   warning ("function not ok for tail call optimization");

This warning may be useful for debugging your changes,
but it is not suitable for inclusion in the FSF's GCC sources.

> ! #define FUNCTION_OK_FOR_SIBCALL(DECL) i386_function_ok_for_sibcall (DECL, exp)

Where did `exp' come from?

Having a macro refer to a value which happens to be in scope like this,
without explicitly passing it or documenting that it must be in scope,
is a very nasty coding style.  That sort of thing would be bound to
cause trouble or confusion later.

It would be better to make `exp' a parameter to FUNCTION_OK_FOR_SIBCALL.

So, for the next iteration:
	- delete the #ifdef __i386 test in calls.c
	- make `exp' a parameter to FUNCTION_OK_FOR_SIBCALL
	- update the documentation for FUNCTION_OK_FOR_SIBCALL in tm.texi
	- update all the definitions of FUNCTION_OK_FOR_SIBCALL
	  for the different targets, to accept (and ignore) the exp
	  parameter, and to return false if fndecl is null
	- update the patch for the gcc-3_4-basic-improvements branch
	- and then repost the patch

I didn't review the changes to the `.md' file.

Cheers,
	Fergus.

P.S.  If you are concerned about not changing the target machine interface,
then another way of handling it would be to add a new target macro
CALL_OK_FOR_SIBCALL(exp) which would be used for indirect calls.
That way the FUNCTION_OK_FOR_SIBCALL macro could remain unchanged,
and the existing non-x86 targets could remain unchanged, you'd just
need to add a default definition for it, and a definition for x86.

In fact thinking about it a bit more, this does seem like a better approach.

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.