Re: [Patch] 1/3 Add new builtin __builtin_flush_icache().

[Richard Sandiford <rsandifo@nildram.co.uk>]

David Daney <ddaney@avtrex.com> writes:
> Richard Sandiford wrote:
>> David Daney <ddaney@avtrex.com> writes:
>>> +  /* Evaluate BEGIN and END. */
>>> +  begin_rtx = expand_expr (begin, NULL_RTX, Pmode, EXPAND_NORMAL);
>>> +  begin_rtx = convert_memory_address (Pmode, begin_rtx);
>>> +
>>> +  end_rtx = expand_expr (end, NULL_RTX, Pmode, EXPAND_NORMAL);
>>> +  end_rtx = convert_memory_address (Pmode, end_rtx);
>>> +
>>> +#ifdef HAVE_clear_cache
>>> +  if (HAVE_clear_cache)
>>> +    emit_insn (gen_clear_cache (begin_rtx, end_rtx));
>>> +#endif
>> 
>> When I said that the switch from general_operand to pmode_register_operand
>> would need target-independent changes, I meant it would need changes to
>> this bit of code.  I think you need to check the predicates of each
>> clear_cache operand and force the operand into a register if it doesn't
>> match.  Something like:
>> 
>>   if (HAVE_clear_cache)
>>     {
>>       icode = CODE_FOR_clear_cache;
>>       if (!insn_data[icode].operand[0].predicate (begin_rtx, Pmode))
>> 	begin_rtx = copy_to_mode_reg (Pmode, begin_rtx);
>>       if (!insn_data[icode].operand[1].predicate (end_rtx, Pmode))
>> 	end_rtx = copy_to_mode_reg (Pmode, end_rtx);
>>       emit_insn (gen_clear_cache (begin_rtx, end_rtx));
>>     }
>> 
>> (It's unfortunate that this construct has to be copied so often,
>> but that's the way it is.)
>> 
>> As it stands, the code only seems to guarantee that the operand
>> is an address operand, whereas the predicates in the MIPS patch
>> claim that it will be a register.
>
> The operands in the MIPS patch are both (match_operand:SI 0 
> "general_operand").  Look at the part in mips.c where I do:
>
> begin = force_reg(Pmode, begin);
>
> If that is the wrong way to put something in a register I will change it.

That's the right way to put something into a register if it isn't
already one.  But I'd like the MIPS pattern to say that it needs
pmode_register_operands from the outset, and for the target-independent
code to force the operands into a register where necessary.  I admit
that not all named patterns work this way -- extv, insv and extzv are
ones that have bitten me in the past -- but the vast majority do.  And I
think that, where possible, we should try to make sure that new patterns
work this way too.

For the vast majority of named patterns, the predicates on the expander
really do meaning something: the code calling the generator function
must make sure that the operands match the predicates first.  With the
patch as it stands, nothing checks the general_operand predicate in the
MIPS clear_cache insn; it's just syntactic lip-service.  The problem is
that it looks (at least to me) like it's more than syntactic lip-service.

Richard