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?

Ciao!
Steven
        * lra.c (check_rtl): Call rtl_profile_for_bb so that recog
        can use cfun->maybe_hot_insn_p.

Index: lra.c
===================================================================
--- lra.c       (revision 204618)
+++ lra.c       (working copy)
@@ -2022,26 +2022,32 @@ check_rtl (bool final_p)
 
   lra_assert (! final_p || reload_completed);
   FOR_EACH_BB (bb)
-    FOR_BB_INSNS (bb, insn)
-    if (NONDEBUG_INSN_P (insn)
-       && GET_CODE (PATTERN (insn)) != USE
-       && GET_CODE (PATTERN (insn)) != CLOBBER
-       && GET_CODE (PATTERN (insn)) != ASM_INPUT)
-      {
-       if (final_p)
-         {
-           extract_insn (insn);
-           lra_assert (constrain_operands (1));
-           continue;
-         }
-       /* LRA code is based on assumption that all addresses can be
-          correctly decomposed.  LRA can generate reloads for
-          decomposable addresses.  The decomposition code checks the
-          correctness of the addresses.  So we don't need to check
-          the addresses here.  */
-       if (insn_invalid_p (insn, false))
-         fatal_insn_not_found (insn);
-      }
+    {
+      /* Whether an insn is recognized or not may depend on the hotness of
+        the insn: Some back ends use it as a layman's "enable" attribute.  */
+      rtl_profile_for_bb (bb);
+
+      FOR_BB_INSNS (bb, insn)
+       if (NONDEBUG_INSN_P (insn)
+           && GET_CODE (PATTERN (insn)) != USE
+           && GET_CODE (PATTERN (insn)) != CLOBBER
+           && GET_CODE (PATTERN (insn)) != ASM_INPUT)
+         {
+           if (final_p)
+             {
+               extract_insn (insn);
+               lra_assert (constrain_operands (1));
+               continue;
+             }
+           /* LRA code is based on assumption that all addresses can be
+              correctly decomposed.  LRA can generate reloads for
+              decomposable addresses.  The decomposition code checks the
+              correctness of the addresses.  So we don't need to check
+              the addresses here.  */
+           if (insn_invalid_p (insn, false))
+             fatal_insn_not_found (insn);
+         }
+    }
 }
 #endif /* #ifdef ENABLE_CHECKING */
 

Reply via email to