[C] : pr52543

Kenneth Zadeck <zadeck@naturalbridge.com> writes:
> +#define FORCE_LOWERING

Don't think you meant to keep this.

> -/* Return whether X is a simple object which we can take a word_mode
> -   subreg of.  */
> +static struct {
> +
> +  /* This pass can transform 4 different operations: move, ashift,
> +     lshiftrt, and zero_extend.  There is a boolean vector for move
> +     splitting that is indexed by mode and is true for each mode that is
> +     to have its copies split.  The other three operations are only done
> +     for one mode so they are only controlled by a single boolean  .*/

Comment out of date.

> +  bool move_modes_to_split[MAX_MACHINE_MODE];
> +  
> +  /* Other splitting operations to be done on the this platform.  */
> +  bool splitting_ashift[MAX_BITS_PER_WORD];
> +  bool splitting_lshiftrt[MAX_BITS_PER_WORD];
> +  bool splitting_zext;
> +
> +  bool something_to_do;
> +
> +  /* Precomputed cost values used to determine if lowering shift
> +     operations is profitable.  */ 
> +  int word_mode_move_cost;
> +  int move_zero_cost;
> +} target_info;

This isn't right.  The point is that there is supposed to be one instance
of this structure per target, along the lines of target_expmed.

Yes, it's clunky.  Hopefully the move to C++ will allow a more elegant
approach.

> +/* Return true if it is profitable to lower and shift by SHIFT_AMT.
> +   CODE can be either ASHIFT or LSHIFTRT.  */
> +static bool
> +profitable_shift_p (enum rtx_code code, int shift_amt)
> +{
> +  rtx trg = gen_rtx_REG (GET_MODE_WIDER_MODE (word_mode), FIRST_PSEUDO_REGISTER);
> +  rtx src = gen_rtx_REG (GET_MODE_WIDER_MODE (word_mode), FIRST_PSEUDO_REGISTER + 1);
> +  int wide_cost 
> +    = insn_rtx_cost (gen_rtx_SET (VOIDmode, trg, 
> +				  gen_rtx_fmt_ee (code, GET_MODE_WIDER_MODE (word_mode), 
> +						  src, GEN_INT (shift_amt))),
> +		     true);

Lines still too long.  (You really need an 80-char-wide editor.
It's the future.)

More importantly, I'd not noticed last time that you're glossing
over the size/speed choice.  That's important, and in general
is made on a per-bb basis.  So I think we need to cache both
the size and speed costs.

> +#ifdef FORCE_LOWERING
> +  return true;
> +#endif
> +
> +#ifdef LOG_COSTS
> +  fprintf (stderr, "shift code %d, shift amt %d, wide_cost %d\n", 
> +	   code, shift_amt, wide_cost);
> +#endif
> +  if (shift_amt == BITS_PER_WORD)
> +    return wide_cost 
> +      >= target_info.word_mode_move_cost + target_info.move_zero_cost;
> +  else
> +    {
> +      int narrow_cost;
> +
> +      trg = gen_rtx_REG (word_mode, FIRST_PSEUDO_REGISTER);
> +      src = gen_rtx_REG (word_mode, FIRST_PSEUDO_REGISTER + 1);
> +      narrow_cost 
> +	= insn_rtx_cost (gen_rtx_SET 
> +			 (VOIDmode, trg, 
> +			  gen_rtx_fmt_ee (code, word_mode, 
> +					  src, 
> +					  GEN_INT (shift_amt - BITS_PER_WORD))),
> +				   true);
> +
> +#ifdef LOG_COSTS
> +      fprintf (stderr, "narrow_cost %d\n", narrow_cost);
> +#endif
> +      return wide_cost > narrow_cost + target_info.move_zero_cost;

Should be ">=".

I think it'd be better to only create the rtx once and simply replace
the shift amount for each test.

> +/* Initialize pass once per execution.  This envolves determining

once per target

> +   which operations on the machine are profitable.  If none are found,
> +   then the pass just returns when called.  */
> +
> +void
> +init_lower_subreg (void)
> +{
> +  rtx trg = gen_rtx_REG (word_mode, FIRST_PSEUDO_REGISTER);
> +  rtx pat;
> +  unsigned int i;
> +  int factor;
> +
> +  memset (&target_info, 0, sizeof target_info);
> +
> +  target_info.word_mode_move_cost = compute_move_cost (word_mode);
> +  target_info.move_zero_cost 
> +    = insn_rtx_cost (gen_rtx_SET (VOIDmode, trg, 
> +				  CONST0_RTX (word_mode)), true);
> +
> +#ifdef LOG_COSTS
> +  fprintf (stderr, "word mode move cost %d\n", target_info.word_mode_move_cost);
> +  fprintf (stderr, "move 0 cost %d\n", target_info.move_zero_cost);
> +#endif
> +  for (i = 0; i < MAX_MACHINE_MODE; i++) 
> +    {
> +      int t;
> +      factor = GET_MODE_SIZE (i) / UNITS_PER_WORD;
> +
> +      if (factor <= 1) 
> +	continue;
> +
> +      t = compute_move_cost ((enum machine_mode) i);
> +
> +#ifdef LOG_COSTS
> +      fprintf (stderr, "mode %s, move cost %d, factor %d\n", 
> +	       mode_name[i], t, factor);
> +#endif
> +      if (t >= target_info.word_mode_move_cost * factor)
> +	{
> +	  target_info.move_modes_to_split[i] = true;
> +	  target_info.something_to_do = true;
> +	}
> +#ifdef FORCE_LOWERING
> +      target_info.move_modes_to_split[i] = true;
> +      target_info.something_to_do = true;
> +#endif
> +    }

The code is a bit inconsistent about whether the costs are calculated
when FORCE_LOWERING is true.  Here you calculate anyway, but in the
shift case you skip.

> +
> +  /* For the moves and shifts, the only case that is checked is one
> +     where the mode of the target is an integer mode twice the width
> +     of the word_mode. 
> +
> +     If it is not profitable to split a double word move then do not
> +     even consider the shifts or the zero extension.  */
> +  if (target_info.move_modes_to_split[(int) GET_MODE_WIDER_MODE (word_mode)])

This should be GET_MODE_2XWIDER_MODE.  Same elsewhere.  (Didn't notice
that last time either, sorry.)

> +    {
> +      /* The only case here to check to see if moving the upper part with a
> +	 zero is cheaper than doing the zext itself.  */
> +      pat = gen_rtx_SET (VOIDmode, trg, 
> +			 gen_rtx_ZERO_EXTEND (GET_MODE_WIDER_MODE (word_mode), 
> +					      gen_reg_rtx (word_mode)));
> +
> +      if (insn_rtx_cost (pat, true) 
> +	  >= target_info.word_mode_move_cost + target_info.move_zero_cost) 
> +	{
> +	  target_info.splitting_zext = true;
> +	  target_info.something_to_do = true;
> +	}

No costs logged here.  Here and in the shifting code, the someting_to_do
assignments should now be redundant.

> +      
> +#ifdef FORCE_LOWERING
> +      target_info.splitting_zext = true;
> +      target_info.something_to_do = true;
> +#endif
> +      /* For the shifts there are three shift amounts that need to be
> +	 checked: word_mode, word_mode + 1 and word_mode * 1.5.  The first
> +	 of these can be done without a shift.  The second and third case
> +	 are the same on large machines but may be different on small
> +	 machines that special case shift 1 and 2.  If any of these are
> +	 found to be useful, then we set the flags to consider those cases
> +	 and when the actual shift amount is known, redo the cost
> +	 calculation.  */

This comment no longer applies.

> @@ -1094,24 +1411,39 @@ decompose_multiword_subregs (void)
>       all the insns.  */
>    {
>      unsigned int i;
> +    bool useful_modes_seen = false;
>  
>      for (i = FIRST_PSEUDO_REGISTER; i < max; ++i)
>        {
> -	if (regno_reg_rtx[i] != NULL
> -	    && GET_MODE_SIZE (GET_MODE (regno_reg_rtx[i])) > UNITS_PER_WORD)
> -	  break;
> +	if (regno_reg_rtx[i] != NULL)
> +	  {
> +	    enum machine_mode mode = GET_MODE (regno_reg_rtx[i]);
> +	    if (regno_reg_rtx[i] != NULL && target_info.move_modes_to_split [mode])

redundant test.

You're probably heartily sick of this patch by now, so here's my
attempt to do the above.  I also made the macros 0/1.  I think
that's generally preferred, because then the conditional code
gets compile testing even when the features are off.  It also
means that LOG_COSTS and FORCE_LOWERING can be turned on at
the same time.

Also, I still think simple_move is the right place to check for
profitable moves.  The comment change I had to make there seems
to reinforce this (the comment was no longer correct either way).

What do you think?  The patch looks OK to me with these changes,
but I'd like to leave it for 48 hours to see if Ian has any comments.

Bootstrapped & regression-tested on x86_64-linux-gnu.  Also tested by
building cc1 on mips64-elf.

Thanks,
Richard

gcc/
2012-04-03  Kenneth Zadeck  <zadeck@naturalbridge.com>
	    Richard Sandiford  <r.sandiford@uk.ibm.com>

	* Makefile.in (lower-subreg.o, target-globals.o): Depend on
	lower-subreg.h.
	* lower-subreg.h: New file.
	* target-globals.h (this_target_lower_subreg): Declare.
	(target_globals): Add lower_subreg;
	(restore_target_globals): Restore this_target_lower_subreg.
	* target-globals.c: Include it.
	(default_target_globals): Add default_target_lower_subreg.
	(save_target_globals): Initialize target_lower_subreg.
	* rtl.h (init_lower_subreg): Added declaration.
	* toplev.c (backend_init_target): Call initializer for lower-subreg
	pass.
	* lower-subreg.c (LOG_COSTS, FORCE_LOWERING): New macros.
	(default_target_lower_subreg): New variable.
	(this_target_lower_subreg): Likewise.
	(twice_word_mode, choices): New macros.
	(shift_cost, compute_splitting_shift, compute_costs)
	(init_lower_subreg): New functions.
	(resolve_simple_move): Add speed_p argument.  Check choices.
	(find_pseudo_copy): Don't check the mode size here.
	(resolve_simple_move): Assert the mode size.
	(find_decomposable_shift_zext): Add speed_p argument and return
	a bool.  Check choices.
	(resolve_shift_zext): Add comment.
	(dump_shift_choices, dump_choices): New functions.
	(decompose_multiword_subregs): Dump list of profitable
	transformations.  Add code to skip non profitable transformations.
	Update calls to simple_move and find_decomposable_shift_zext.

[C] : pr52543

Commit Message

Comments

Patch