Re: Cleaning up expand optabs code

[Richard Henderson <rth at redhat dot com>]

On 03/17/2011 09:32 AM, Richard Sandiford wrote:
> This patch adds a few helper functions for dealing with optabs:
> 
> - insn_operand_matches (ICODE, OPNO, X)
> - (maybe_)legitimize_insn_target (ICODE, OPNO, TARGET, MODE)
> - (maybe_)legitimize_insn_source (ICODE, OPNO, SOURCE, MODE)

Cool.

Why pass in MODE in the later two cases?  Seems to me that your
argument for omitting it for insn_operand_matches is just as
compelling for the other functions...

> - I first tried to make insn_operand_matches an inline function,
>   but I can't think of a good header file that is guaranteed to
>   know about both recog.h and insn-codes.h.

Meh.  I can't imagine it mattering so much.  Anyway, an lto build
of gcc itself would fix that if needed, right?  ;-)

> -      char_rtx = const0_rtx;
> -      char_mode = insn_data[(int) icode].operand[2].mode;
> -      if (! (*insn_data[(int) icode].operand[2].predicate) (char_rtx,
> -							    char_mode))
> -	char_rtx = copy_to_mode_reg (char_mode, char_rtx);
> -
> -      pat = GEN_FCN (icode) (result, gen_rtx_MEM (BLKmode, src_reg),
> -			     char_rtx, GEN_INT (align));
> -      if (! pat)
> -	return NULL_RTX;
> +      if (!(char_rtx = maybe_legitimize_insn_source (icode, 2, const0_rtx,
> +						     VOIDmode))
> +	  || !(pat = GEN_FCN (icode) (result, gen_rtx_MEM (BLKmode, src_reg),
> +				      char_rtx, GEN_INT (align))))
> +	{
> +	  delete_insns_since (before_strlen);
> +	  return NULL_RTX;
> +	}
>        emit_insn (pat);

I'm not so fond of burying assignments into conditionals.  I know we
do it a lot in gcc, and it's a tad harder to avoid here than ...

> -      if (!insn_data[icode].operand[1].predicate (val, mode))
> -	val = force_reg (mode, val);
> -
> -      insn = GEN_FCN (icode) (mem, val);
> -      if (insn)
> +      start = get_last_insn ();
> +      if ((val = maybe_legitimize_insn_source (icode, 1, const0_rtx, mode))
> +	  && (insn = GEN_FCN (icode) (mem, val)))
>  	{
>  	  emit_insn (insn);
>  	  return;
>  	}
> +      delete_insns_since (start);

... here, where it's just as readable to write

	val = maybe_legitimize_insn_source (icode, 1, const0_rtx, mode);
	if (val)
	  {
	    insn = GEN_FCN (icode) (mem, val);
	    if (insn)
	      {
		emit_insn (insn);
		return;
	      }
	  }
	delete_insns_since (start);

> +  if ((temp = maybe_legitimize_insn_target (icode, 0, target, tmp_mode))
> +      && (xop0 = maybe_legitimize_insn_source (icode, 1, xop0, mode0))
> +      && (xop1 = maybe_legitimize_insn_source (icode, 2, xop1, mode1))
> +      && (pat = GEN_FCN (icode) (temp, xop0, xop1)))

Although, these patterns occur often enough that I wonder if there's a way
to tidy them up into a single function call.  We could either use a set of
functions like

    target = maybe_legitimize_emit_insn_ts (icode, target, src1);
    target = maybe_legitimize_emit_insn_tss (icode, target, src1, src2);

or have a single function with variadic source operands.  Probably the
separate functions is better for error checking within the compiler.  We
can validate the correct function was called for a given icode based on
the n_operands stored in the insn_data, and the compiler itself will make
sure that we didn't omit an argument for that function.

The interface I was considering here would validate the target and all
of the sources, emit the insn or delete_insns_since, and return the new
target or NULL if no insn was emitted.

All that said, I'm very much in favour of this cleanup.


r~