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.guent...@gmail.com> wrote: > Steven Bosscher <stevenb....@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 > >
