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~
