Hi,
  This patch replaces rtx_cost with insn_cost in forward propagation.
In the PR, one constant vector should be propagated and replace a
pseudo in a store insn if we know it's a duplicated constant vector.
It reduces the insn cost but not rtx cost. In this case, the cost is
determined by destination operand (memory or pseudo). Unfortunately,
rtx cost can't help.

  The test case is added in the second rs6000 specific patch.

  Compared to previous version, the main changes are:
1. Invoke change_is_worthwhile to judge if the cost is reduced and
the replacement is worthwhile.
2. Invalidate recog data before getting the insn cost for the new
rtl as insn cost might call extract_constrain_insn_cached and
extract_insn_cached to cache the recog data. The cache data is
invalid for the new rtl and it causes ICE.
3. Check if the insn cost of new rtl is zero which means unknown
cost. The replacement should be rejected at this situation.

Previous version
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651233.html

  The patch causes a regression cases on i386 as the pattern cost
regulation has a bug. Please refer the patch and discussion here.
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651363.html

  Bootstrapped and tested on powerpc64-linux BE and LE with no
regressions. Is it OK for the trunk?

ChangeLog
fwprop: invoke change_is_worthwhile to judge if a replacement is worthwhile

gcc/
        * fwprop.cc (try_fwprop_subst_pattern): Invoke change_is_worthwhile
        to judge if a replacement is worthwhile.
        * rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Invalidate
        recog data before getting the insn cost for the new rtl.  Check if
        the insn cost of new rtl is unknown and fail the replacement.

patch.diff
diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
index de543923b92..975de0eec7f 100644
--- a/gcc/fwprop.cc
+++ b/gcc/fwprop.cc
@@ -471,29 +471,19 @@ try_fwprop_subst_pattern (obstack_watermark &attempt, 
insn_change &use_change,
       redo_changes (0);
     }

-  /* ??? In theory, it should be better to use insn costs rather than
-     set_src_costs here.  That would involve replacing this code with
-     change_is_worthwhile.  */
   bool ok = recog (attempt, use_change);
-  if (ok && !prop.changed_mem_p () && !use_insn->is_asm ())
-    if (rtx use_set = single_set (use_rtl))
-      {
-       bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (use_rtl));
-       temporarily_undo_changes (0);
-       auto old_cost = set_src_cost (SET_SRC (use_set),
-                                     GET_MODE (SET_DEST (use_set)), speed);
-       redo_changes (0);
-       auto new_cost = set_src_cost (SET_SRC (use_set),
-                                     GET_MODE (SET_DEST (use_set)), speed);
-       if (new_cost > old_cost
-           || (new_cost == old_cost && !prop.likely_profitable_p ()))
-         {
-           if (dump_file)
-             fprintf (dump_file, "change not profitable"
-                      " (cost %d -> cost %d)\n", old_cost, new_cost);
-           ok = false;
-         }
-      }
+  if (ok && !prop.changed_mem_p () && !use_insn->is_asm ()
+      && single_set (use_rtl))
+    {
+      if (!change_is_worthwhile (use_change, false)
+         || (!prop.likely_profitable_p ()
+             && !change_is_worthwhile (use_change, true)))
+       {
+         if (dump_file)
+           fprintf (dump_file, "change not profitable");
+         ok = false;
+       }
+    }

   if (!ok)
     {
diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
index 11639e81bb7..9bad6c2070c 100644
--- a/gcc/rtl-ssa/changes.cc
+++ b/gcc/rtl-ssa/changes.cc
@@ -185,7 +185,18 @@ rtl_ssa::changes_are_worthwhile (array_slice<insn_change 
*const> changes,
                              * change->old_cost ());
       if (!change->is_deletion ())
        {
+         /* Invalidate recog data as insn_cost may call
+            extract_insn_cached.  */
+         INSN_CODE (change->rtl ()) = -1;
          change->new_cost = insn_cost (change->rtl (), for_speed);
+         /* If the cost is unknown, replacement is not worthwhile.  */
+         if (!change->new_cost)
+           {
+             if (dump_file && (dump_flags & TDF_DETAILS))
+               fprintf (dump_file,
+                        "Reject replacement due to unknown insn cost.\n");
+             return false;
+           }
          new_cost += change->new_cost;
          if (for_speed)
            weighted_new_cost += (cfg_bb->count.to_sreal_scale (entry_count)

Reply via email to