reworked long_call heuristic patch.

Nick Clifton nickc@cygnus.com
Tue Feb 22 14:34:00 GMT 2000


Hi Dimitri,

: I'm sending the reworked patch.

Thanks for the resubmisison.  Unfortunately I am not accepting this
patch in its current state, but I think that with a little work it can
become acceptable.  See below...

: However now there are still at least two 'weak' points which I'm not
: quite sure about.  First, in gcc/tree.c and gcc/cp/tree.c when new
: function or method types are being built, hash table is checked for
: existence of the same type and if found the tree_type pointer of the
: function declaration is set to point to the found tree node.  Thus
: there can be many function declaration tree nodes that share the same
: tree_type nodes.  It is a problem when type attributes need to be set.
: Assigning attributes to such shared type breaks the long-call
: heuristic.  For now I parenthesized all the hash table checks in #if
: 0/#endif, thus making every fundecl tree node have its own copy of
: corresponding tree_type node.  I don't think this is a correct
: solution. But I don't think that decls should share type nodes either.

Hmm, well I would be very loath to change the current behaviour of the
compiler in this regard.  There may well be very good reasons for
sharing these type nodes.  Instead I would recommend a different way
of solving this problem - do not attach attributes to the function's
type.  Instead change the name of the function!  This is how other
ports solve this problem.  (For example see the PowerPC or the V850
ports).  What they do is encode attribute like information in the
function name by appending an at sign '@' followed by some special
codes.  There are macros that can then be defined to strip this
encoding off the name before it used by the compiler, so that they do
not affect the rest of the compilaiton process.

: Second I'm assigning short_call attributes to functions declared
: static from ENCODE_SECTION_INFO.  I'm not sure whether this is the
: correct place to do this.

Actually this is a reasonable place to do this.

: three arm.[hc]/md files.  One more thing -- arm_comp_type_attributes
: doesn't work at all.  With current arm_comp_type_attributes compiler
: rejects all __attribute__ syntax,

Can you explain why this is so ?  Looking at the current code, it
certainly appears to work to me.

: Finally, I noticed last time you applied the patch without updating
: *.texi files.  If this is intentional and the changes should be
: reworked, please let me know.  In any case I assume the long-call
: support in ARM should be documented.

You are correct.  This was a mistake on my part.

: I guess no new ChangeLog entry is needed with this patch...

Well it is always nice to have a Changelog entry incldued with a
patch.  A lot of people who read the gcc-patches mailing list look at
the ChangeLog first, in order to decide if they are interested in
looking at the body of the patch.  And besides if I had decided to
accept the patch, I would have then needed to go the archives, find
the previous patch, extract the ChangeLog, modify to reflect the
changes that you have made, etc, and I am a very lazy kind of guy! :-)

And now some specific comments on parts of the patch:

: + Do calls indirect with loading of 32-bit address of the callee into
: + @samp{PC} register.  You need to use this switch, if you call outside of
: + the current 64 megabyte segment to functions that are not through
: + pointers.  Not all calls made indirect when this option is used.

You are missing a verb in the final sentance above.

: + The heuristic is that functions declared static and also the
: + functions which definitions have been seen in the same source
: + file, before the call in question being parsed, are close enough
: + and need not be called indirectly.

I would suggest rewording this sentance to:

  "The heuristic is that static functions, and functions whoes
  definitions have already been compiled within the current
  compilation unit, are considered to be close enough, and will not be
  called indirectly.  The exceptions to this are weak function
  defintions and functions whoes section are explicitly specified."

: diff -c -3 -p -r1.68.2.20 arm.c

I would recommend adding the "-w" command line switch when you run
diff.  This will skip differences in white space which will avoid
including entries like this in the patch:

: *** arm.c	2000/02/10 13:59:01	1.68.2.20
: --- arm.c	2000/02/19 02:20:45
: *************** arm_override_options ()
: *** 421,427 ****
:   	}
:         else if (! TARGET_APCS_32)
:   	sought |= FL_MODE26;
: !       
:         if (sought != 0 && ((sought & insn_flags) != sought))
:   	{
:   	  /* Try to locate a CPU type that supports all of the abilities
: --- 421,427 ----
:   	}
:         else if (! TARGET_APCS_32)
:   	sought |= FL_MODE26;
: ! 
:         if (sought != 0 && ((sought & insn_flags) != sought))
:   	{
:   	  /* Try to locate a CPU type that supports all of the abilities

Coding style:

: !   /* check for long_call/short_call attribute  */

The GNU coding standard says that comments should be treated as
English sentances.  They should start with a capital letter, end with
a period, and there should be two spaces between the final period and
the closing comment marker.

: *************** arm_function_arg (pcum, mode, type, name
: *** 1578,1592 ****
:        int named;
:   {
:     if (mode == VOIDmode)
: !     /* Compute operand 2 of the call insn.  */
: !     return GEN_INT (pcum->long_call);
: !   
: --- 1586,1607 ----
:        int named;
:   {
:     if (mode == VOIDmode)
: !     {
: !       /* Compute operand 2 of the call insn.  */
: !       return GEN_INT (pcum->long_call);
: !     }
: ! 

The GNU coding standard says that curly braces should be omitted when
a block only contains a single statement.

: *************** arm_valid_type_attribute_p (type, attrib
: *** 1619,1624 ****
: --- 1634,1640 ----
:        tree identifier;
:        tree args;
:   {
: + 
:     if (   TREE_CODE (type) != FUNCTION_TYPE
:         && TREE_CODE (type) != METHOD_TYPE
:         && TREE_CODE (type) != FIELD_DECL

What is this extra line doing here ?


: + } /* arm_set_default_type_attributes */

None of the other functions in this file have their name repeated in a
comment at the end.  Whilst this is not really a problem it does seem
a little bit odd.

: +   if (sym_ref == XEXP (DECL_RTL (current_function_decl), 0))
: +     {
: +       return 0;
: +     }

You must also check to make sure that the current function definition
is not weak.  ie "! DECL_WEAK (symref)"

Also what happened to calling 'current_file_function_operand' here ?
Having a seperate function (whoes heuristic can always be improved by
later patches) seems like a good idea to me.

: + #else
: + /* If we are referencing a function that is static or is known to be 
: +    in this file attach short_call attribute to the type associated
: +    with this declaration.  The same is for #ifndef AOF_ASSEMBLER
: +    above. */
: + #define ENCODE_SECTION_INFO(decl)					\
: + {									\
: +   if (TREE_CODE (decl) == FUNCTION_DECL	&& ! TREE_PUBLIC (decl))        \
: +     arm_set_short_call_attribute (decl);				\
: + }
:   #endif

Another place to check for DECL_WEAK.

Cheers
	Nick


More information about the Gcc-patches mailing list