Cleaning up expand optabs code

Richard Henderson <rth@redhat.com> writes:
> 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)
>
> 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...

The problem is that the insn_data mode isn't always going to tell us
what mode a new register should have.  match_operands without modes are
only a warning, not an error, and they're sometimes needed.  E.g. extv
and insv get passed word_mode values for registers and QImode values
for memories.  And even the register case is a problem, because word_mode
isn't constant on some targets.

We also have patterns like prefetch that take Pmode operands, and Pmode
is again not constant on some targets.  This case matters because we can
in principle have CONST_INT addresses that need to be forced into
registers.

Then there are special predicates, where the predicate mode in the
insn_data isn't necessarily the mode of the operand.  This again
matters for address operands, because they could be CONST_INTs
that need to be forced into registers.

I agree it'd be better if we could know from insn_data what the
right mode is for generation as well as matching, but it'd be
a lot more work.  Which I suppose is a long-winded way of saying
I bottled it.

>> - 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?  ;-)

Of course :-)

>> -      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 ...

Yeah, I wasn't too fond of that either to be honest.

>> +  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.

OK, I gave that a go, but in the end, it seemed that there were
just too many variations for the _ts, _tss, etc.  It might not be
too bad if it was just "target" and "source", but many callers
want "fixed" as well.

> 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.

Yeah, I agree a higher-level interface like that would be better.
In the end I went for a function that takes an array of light-weight
structures that describe each operand.  This also makes it easier to
handle other common idioms, like calling convert_modes beforehand.

Given the mode stuff above, I've tried to be quite draconian as far
as caller-provided modes go.  I think the caller really should know
what mode they're dealing with.  The one case where I had to hold
back a bit was create_convert_operand_from, which replaces things like:

  if (GET_MODE (op0) != mode0 && mode0 != VOIDmode)
    xop0 = convert_modes (mode0,
                          GET_MODE (op0) != VOIDmode
                          ? GET_MODE (op0)
                          : mode,
                          xop0, unsignedp);

It seems a little suspicious that we only trust "mode" for CONST_INT
op0s, and not for other cases, but I'd like to leave that for now.
Maybe a future "clean up"?

Also, I had to change the i386 setmem pattern in order to avoid
a regression in cold-attribute-1.c.  The current predicate for
the character operand is "const_int_operand", but the pattern
handles registers as well.  I'm sure there are going to be other
things like that, so sorry in advance if this patch goes in and
breaks a target...

Anyway, thanks for pushing back on this.  Looking back, the original
interface was a bit poor.  I hope the new version really is cleaner,
rather than just a token wiping over with suds.

Bootstrapped & regression-tested on x86_64-linux-gnu.

Richard

gcc/
	* expr.h (prepare_operand): Move to optabs.h.
	* optabs.h (emit_unop_insn): Change insn code parameter from "int"
	to "enum insn_code".
	(maybe_emit_unop_insn): Likewise.
	(prepare_operand): Likewise.  Move from expr.h.
	(MAX_EXPAND_OPERANDS): New macro.
	(expand_operand_type): New enum.
	(expand_operand): New structure.
	(create_expand_operand): New function.
	(create_fixed_operand, create_output_operand): Likewise
	(create_input_operand, create_convert_operand_to): Likewise.
	(create_convert_operand_from, create_address_operand): Likewise.
	(create_integer_operand, make_operand_commutative): Likewise.
	(create_convert_operand_from_type, insn_operand_matches)
	(maybe_legitimize_operands, maybe_gen_insn, maybe_expand_insn)
	(maybe_expand_jump_insn, expand_insn, expand_jump_insn): Declare.
	* builtins.c (expand_builtin_prefetch): Use the new interfaces.
	(expand_builtin_interclass_mathfn, expand_builtin_strlen): Likewise.
	(expand_movstr, expand_builtin___clear_cache): Likewise.
	(expand_builtin_lock_release): Likewise.
	* explow.c (allocate_dynamic_stack_space): Likewise.
	(probe_stack_range): Likewise.  Allow check_stack to FAIL,
	and use the default handling in that case.
	* expmed.c (check_predicate_volatile_ok): Delete.
	(store_bit_field_1, extract_bit_field_1): Use the new interfaces.
	(emit_cstore): Likewise.
	* expr.c (init_expr_target, emit_block_move_via_movmem): Likewise.
	(set_storage_via_setmem, compress_float_constant): Likewise.
	(expand_assignment, emit_storent_insn, try_casesi): Likewise.
	(emit_single_push_insn): Likewise.  Allow the expansion to fail.
	* function.c (safe_insn_predicate): Use the new interfaces.
	(assign_parm_setup_reg): Likewise.
	* optabs.c (expand_widen_pattern_expr, expand_ternary_op): Likewise.
	(expand_vec_shift_expr, expand_binop_directly): Likewise.
	(expand_twoval_unop, expand_twoval_binop): Likewise.
	(expand_unop_direct, prepare_cmp_insn): Likewise.
	(emit_cmp_and_jump_insn_1, emit_indirect_jump): Likewise.
	(emit_conditional_move, gen_add2_insn, gen_add3_insn): Likewise.
	(have_add2_insn, gen_sub2_insn, have_sub2_insn): Likewise.
	(gen_cond_trap, vector_compare_rtx, expand_vec_cond_expr): Likewise.
	(expand_val_compare_and_swap_1, expand_sync_operation): Likewise.
	(expand_sync_fetch_operation, expand_sync_lock_test_and_set): Likewise.
	(maybe_emit_unop_insn): Likewise.  Change icode to an insn_code.
	(emit_unop_insn): Likewise.
	(expand_copysign_absneg, prepare_operand): Change icode to an insn_code.
	(create_convert_operand_from_type): New function.
	(insn_operand_matches, maybe_legitimize_operand): Likewise.
	(maybe_legitimize_operands, maybe_gen_insn): Likewise.
	(maybe_expand_insn, maybe_expand_jump_insn): Likewise.
	(expand_insn, expand_jump_insn): Likewise.
	* reload.c (find_reloads_address_1): Use insn_operand_matches.
	* reload1.c (gen_reload): Likewise.
	* targhooks.c (default_secondary_reload): Likewise.
	* config/i386/i386.md (setmem<mode>): Use nonmemory_operand rather
	than const_int_operand for operand 2.

Cleaning up expand optabs code

Commit Message

Comments

Patch