Re: [patch][RFC] make lra.c:check_rtl set maybe_hot_insn_p

[Yvan Roux <yvan dot roux at linaro dot org>]

Hi,

I'm agree.  I looked at the ARM backend and it occurs that the usage
of optimize_insn_for_size_p() was added to only use store_minmax in
cold path because of some performance issue.  But in any case its
usage doesn't shrink the number of instruction, if we are in ARM mode
3 are needed : 1 comparison, 1 conditional move and 1 store in O1, O2
and O3 and 1 comparison and 2 conditional sotres in Os :

cmp     ip, r0
movlt   r0, ip
str        r0, [lr, r3]

or

cmp   r5, r4
strle   r5, [lr, r3]
strgt   r4, [lr, r3]

and in Thumb mode 4 are needed in both cases because of the it
insertion.  Thus I think that this insn can be removed. Is it ok for
you ?

Thanks
Yvan


On 10 November 2013 14:17, Richard Biener <richard.guenther@gmail.com> wrote:
> Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>>Hello,
>>
>>This patch is necessary to make ARM pass the test suite with LRA
>>enabled. The symptom is recog failing to recognize a store_minmaxsi
>>insn, see:
>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00725.html
>>
>>But I am not sure if that's also the root cause of the problem, or
>>whether the ARM back end should not let recognition of insn patterns
>>be dependent on the state of the profile flags.
>>
>>The pattern for store_minmaxsi (in arm.md) is:
>>
>>(define_insn "*store_minmaxsi"
>>  [(set (match_operand:SI 0 "memory_operand" "=m")
>>        (match_operator:SI 3 "minmax_operator"
>>         [(match_operand:SI 1 "s_register_operand" "r")
>>          (match_operand:SI 2 "s_register_operand" "r")]))
>>   (clobber (reg:CC CC_REGNUM))]
>>  "TARGET_32BIT && optimize_insn_for_size_p()"
>>  "*
>>  operands[3] = gen_rtx_fmt_ee (minmax_code (operands[3]), SImode,
>>                                operands[1], operands[2]);
>>  output_asm_insn (\"cmp\\t%1, %2\", operands);
>>  if (TARGET_THUMB2)
>>    output_asm_insn (\"ite\t%d3\", operands);
>>  output_asm_insn (\"str%d3\\t%1, %0\", operands);
>>  output_asm_insn (\"str%D3\\t%2, %0\", operands);
>>  return \"\";
>>  "
>>  [(set_attr "conds" "clob")
>>   (set (attr "length")
>>        (if_then_else (eq_attr "is_thumb" "yes")
>>                      (const_int 14)
>>                      (const_int 12)))
>>   (set_attr "type" "store1")]
>>)
>>
>>
>>Note the insn condition uses optimize_insn_for_size_p(). This means
>>the pattern can be valid or invalid dependent on the context where the
>>insn appears: in hot or cold code. IMHO this behavior should not be
>>allowed. The back end cannot expect the middle end to know at all
>>times the context that the insn appears in, and more importantly
>>whether a pattern is valid or not is independent of where the insn
>>appears: That is a *cost* issue!
>>
>>It seems to me that the ARM back end should be fixed here, not LRA's
>>check_rtl.
>>
>>Comments&thoughts?
>
> The intent is to allow this also to control combine and split, but certainly making insns invalid after the fact is bad.  think of sinking a previously hot insn into a cold path...
>
> Honza, how was this supposed to work?
>
> Richard.
>
>>Ciao!
>>Steven
>
>