Re: PATCH for sibcalls on i386

[Andreas Bauer <baueran at in dot tum dot de>]

Hi Fergus (and others),

Thank you a lot for the comments.

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

Ok.

> 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.

That check is already extant in FUNCTION_OK_FOR_SIBCALL, for all targets,
as far as I know.  The reason why fndecl is being checked in calls.c
again is that a few other macros expect it to have a sensible value.

> > +       /* 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.

Ouch.  I could have checked it, but didn't think of it.  :-(

I think, I was indeed to tentative here and should have removed this
check and the conditions around it at once.  Didn't occur to me at the
time.

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

Obviously, I did not expect the patch to be approved as is.  I was aware
that some lines in it were rather naughty and that's the reason why I was
looking for advice.  Of course, such warnings would _not_ be part of
anything "official" and if people mind them, will be removed from future
patches/discussions.

> > ! #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.

...which means having to change it for _all_ the targets gcc supports.
Again, I was too tentative and wanted to discuss first, before I do
something like this.

> So, for the next iteration:
> 	- delete the #ifdef __i386 test in calls.c

Ok.

> 	- 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

Ok, this is a rather huge change.  Will I feed each target as an
individual patch?

> 	- update the patch for the gcc-3_4-basic-improvements branch

Ok.

> 	- and then repost the patch

Will happily do so.

Cheers,
Andi.