Re: Fix for Thumb compiling va-arg-11.c

[Richard Earnshaw <rearnsha at arm dot com>]

I was looking at this same problem on Monday night, but didn't get as far 
as fixing it.  My thoughts were as follows:

1) The basic problem is as you describe it... :-)

2) Yes, we need to cache the fact that we have at one point said that a 
far_jump is in used.

3) We only need to use this if we have eliminated afp into the stack 
pointer or the frame pointer: that is, afp was a live register before 
reload ran -- if the function doesn't have any arguments on the stack, 
then it doesn't matter whether or not we subsequently eliminate the push 
of lr, because there are no offsets dependent on this.

Using this method there would still be a small number of functions where 
we would unnecessarily push lr in the prologue, but they would be very 
rare.  I doubt that we would really find many cases where we were 
pessimistic.

Determining that afp was live before reload should be fairly straight 
forward (and probably quicker than testing the size of the function) -- 
something in INITIAL_ELIMINATION_OFFSET like

	afp_was_live |= regs_ever_live[afp_regnum]

should do the trick (we could clear this at the end of each function).  
The OR is important since once we have eliminated afp into a real register 
it won't be marked live any more (I think).

The final part, of course, is that we always push lr in the prologue if 
afp_was_live is true and the code potentially contains a far_jump.

R.

PS.  Your current patch has a bug:  it doesn't correctly take into account 
the size of addr_vecs.  More importantly, until the constant pools have 
been inserted (which isn't until after reload has completed), you really 
don't know how large the function will be: it could be that a branch is in 
range during reload, but that we then push a constant pool in the middle 
and it moves out of range.

> Hi Guys,
> 
>   I would like to apply the following patch to fix a problem with the
>   Thumb compiler.  The problem, which was detected by the gcc
>   testsuite case execute/va-arg-11.c, is that the predicate
>   thumb_far_jump_used_p() can return true during the early stages of a
>   comilation, but then later change its mind and return false during
>   the later stages.
> 
>   The reason is that the predicate detects the presence of the
>   far_jump attribute attached to a jump insn.  Thsi attribute is
>   dependent upon the length of the insn, which in turn is dependent
>   upon the distance between the jump and its target.  During the early
>   stages of the compilation, this distance is not known, so gcc
>   defaults to returning the longest possible length, which sets the
>   far jump attribute.
> 
>   Later on, the relative locations of the insns are fixed, so the
>   length of the jump insn can be calculated properly, and so the
>   far_jump attribute can be set correctly.
> 
>   The problem with this behaviour is that for leaf functions, if a far
>   jump is going to be generated, then a stack slot has to be assigned
>   to hold the link register.  (The link register is used to hold the
>   destination address of the far jump).  Unfortunately the decision
>   about whether the link register is going to be stored on the stack
>   has to be made before the far jump attributes can be correctly
>   calculated, so gcc has to guess.
> 
>   The bug was that gcc would guess that the link register would be
>   used, assign stack slots and then later on, when the prologue and
>   epilogue code was being generated, the guess would be overridden,
>   the link register would not be saved, but know all of the stack
>   assignments were out of place.
> 
>   The patch fixes this problem in two ways.  Firstly if
>   thumb_far_jump_used_p () does return true, it remembers this fact and
>   will henceforth always return true (for the function being compiled
>   at that time).  Secondly it adds a second heuristic to the code so
>   that even if a far jump attributed insn is detected in the function,
>   it will not return true, unless there are enough insns in the
>   function to make a far jump an actual possibility.
> 
>   I am not sure if this solution is the best way to fix this problem,
>   but it does work and it doesn;t introduce any new failures, so it
>   seems like a good idea to me.
> 
>   I have generated this patch against the merged arm/thumb branch
>   although a very similar version can be applied to the egcs mainlinbe
>   code as well.
> 
> Cheers
> 	Nick
> 
> 2000-03-21  Nick Clifton  <nickc@cygnus.com>
> 
> 	* config/strongarm2/arm.c (struct machine_function): Add new
> 	field 'far_jump_used'.
> 	(thumb_far_jump_used_p): Cache result in far_jump_used.
> 	Do not return true if there are not enough instructions in the
> 	function to warrant a far jump.
> 
> Index: arm.c
> ===================================================================
> RCS file: /cvs/gcc/egcs/gcc/config/arm/arm.c,v
> retrieving revision 1.68.2.23
> diff -p -r1.68.2.23 arm.c
> *** arm.c	2000/03/15 22:07:12	1.68.2.23
> --- arm.c	2000/03/21 23:19:51
> *************** char * arm_condition_codes[] =
> *** 205,214 ****
>     "hi", "ls", "ge", "lt", "gt", "le", "al", "nv"
>   };
>   
> ! /* Data for propagating the return address.  */
>   struct machine_function
>   {
> !   rtx ra_rtx;
>   };
>   
>   #define streq(string1, string2) (strcmp (string1, string2) == 0)
> --- 205,215 ----
>     "hi", "ls", "ge", "lt", "gt", "le", "al", "nv"
>   };
>   
> ! /* Per function data.  */
>   struct machine_function
>   {
> !   rtx ra_rtx;		/* Records return address.  */
> !   int far_jump_used;	/* Records if LR has to be saved for far jumps.  */
>   };
>   
>   #define streq(string1, string2) (strcmp (string1, string2) == 0)
> *************** thumb_shiftable_const (val)
> *** 8290,8310 ****
>     return 0;
>   }
>   
> ! /* Returns non-zero if the current function contains a far jump */
>   int
>   thumb_far_jump_used_p (void)
>   {
>     rtx insn;
>     
>     for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
>       {
>         if (GET_CODE (insn) == JUMP_INSN
>   	  /* Ignore tablejump patterns.  */
>   	  && GET_CODE (PATTERN (insn)) != ADDR_VEC
>   	  && GET_CODE (PATTERN (insn)) != ADDR_DIFF_VEC
>   	  && get_attr_far_jump (insn) == FAR_JUMP_YES
>   	  )
> ! 	return 1;
>       }
>   
>     return 0;
> --- 8291,8359 ----
>     return 0;
>   }
>   
> ! /* Returns non-zero if the current function contains a far jump.  */
>   int
>   thumb_far_jump_used_p (void)
>   {
>     rtx insn;
> +   unsigned int insn_bytes;
>     
> +   /* If we have already decided that far jumps may be used,
> +      do not bother checking again.  */
> +   if (cfun->machine->far_jump_used)
> +     return 1;
> +   
> +   /* We used to just look for jump instructions with the far_jump
> +      attribute set.  Unfortunately this attribute is based on the length
> +      of the insn, and this length is computed by an alogirthm based on
> +      the distance between the jump instruction and its target.  Since
> +      this distance information is only available very close to the end
> +      of the compile process, most of the times when this function is
> +      called, gcc will have to guess at the length.  It defaults to
> +      choosing the largest possible length, which in turns sets the
> +      far_jump attribute to true.
> + 
> +      This resulted in many false negatives, since any function which
> +      contained a branch instruction would generate a 'true' result.
> +      This causes problems for the stack layout code, since it has to
> +      decide relatively early on whether to save a stack slot for the LR
> +      register which is used by far_jumps.  So a lot of the time in leaf
> +      functions the LR register would be saved unnecessarily.
> + 
> +      In order to work around this problem this code now also calculates
> +      the total potential length of the function, and only returns true
> +      if it contains a 'far_jump' type branch and there enough instructions
> +      to make it possible that a far jump will actually be needed.  */
> +   insn_bytes = 0;
> +   
>     for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
>       {
> +       insn_bytes += get_attr_length (insn);
> +       
>         if (GET_CODE (insn) == JUMP_INSN
>   	  /* Ignore tablejump patterns.  */
>   	  && GET_CODE (PATTERN (insn)) != ADDR_VEC
>   	  && GET_CODE (PATTERN (insn)) != ADDR_DIFF_VEC
>   	  && get_attr_far_jump (insn) == FAR_JUMP_YES
>   	  )
> ! 	break;
> !     }
> ! 
> !   if (insn)
> !     {
> !       /* We found a least one candidate for a far jump instruction.
> ! 	 Now just finish off computing how long this function may be
> ! 	 and then decide if it is long enough to warrant supposing
> ! 	 that a far jump may be needed.  */
> !       for (insn = NEXT_INSN (insn); insn; insn = NEXT_INSN (insn))
> ! 	insn_bytes += get_attr_length (insn);
> ! 
> !       if (insn_bytes >= 2050)
> ! 	{
> ! 	  /* Record this decision in the machine structure.  */
> ! 	  cfun->machine->far_jump_used = 1;
> ! 	  return 1;
> ! 	}
>       }
>   
>     return 0;