This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition


On 11/10/14 13:44, Felix Yang wrote:
Hello Jeff,

     I see that you have improved the RTL typesafety issue for ira.c,
so I rebased this patch
     on the latest trunk and change to use the new list walking interface.
     Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
     OK for trunk?
Hi Felix,
I believe your patch causes a regression for arm-none-eabi.
FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2

This happens because your patch stops reuse of code for
" return -1;" statements in pr43920-2.c.

As far as I investigated, your patch prevents adding "(expr_list (-1) (nil)" in ira pass, which prevents jump2 optimization from happening.

So before, in ira pass I could see:
"(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
(const_int -1 [0xffffffffffffffff])) /work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 613 {*thumb2_movsi_vfp}
     (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
        (nil)))"
But with your patch I get
"(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
(const_int -1 [0xffffffffffffffff])) /work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 615 {*thumb2_movsi_vfp}
     (nil))"

This causes a code generation regression and needs to be fixed.
Kind regards,
Alex


Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog    (revision 216116)
+++ gcc/ChangeLog    (working copy)
@@ -1,3 +1,14 @@
+2014-10-11  Felix Yang  <felix.yang@huawei.com>
+        Jeff Law  <law@redhat.com>
+
+    * ira.c (struct equivalence): Change member "is_arg_equivalence"
and "replace"
+    into boolean bitfields; turn member "loop_depth" into a short
integer; add new
+    member "no_equiv" and "reserved".
+    (no_equiv): Set no_equiv of struct equivalence if register is marked
+    as having no known equivalence.
+    (update_equiv_regs): Check all definitions for a multiple-set
+    register to make sure that the RHS have the same value.
+
  2014-10-11  Martin Liska  <mliska@suse.cz>

      PR/63376
Index: gcc/ira.c
===================================================================
--- gcc/ira.c    (revision 216116)
+++ gcc/ira.c    (working copy)
@@ -2902,12 +2902,14 @@ struct equivalence

    /* Loop depth is used to recognize equivalences which appear
       to be present within the same loop (or in an inner loop).  */
-  int loop_depth;
+  short loop_depth;
    /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
    /* Set when an attempt should be made to replace a register
       with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
  };

  /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3255,6 +3257,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
    if (!REG_P (reg))
      return;
    regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
    list = reg_equiv[regno].init_insns;
    if (list && list->insn () == NULL)
      return;
@@ -3381,7 +3384,7 @@ update_equiv_regs (void)

        /* If this insn contains more (or less) than a single SET,
           only mark all destinations as having no known equivalence.  */
-      if (set == 0)
+      if (set == NULL_RTX)
          {
            note_stores (PATTERN (insn), no_equiv, NULL);
            continue;
@@ -3476,16 +3479,49 @@ update_equiv_regs (void)
        if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
          note = NULL_RTX;

-      if (DF_REG_DEF_COUNT (regno) != 1
-          && (! note
+      if (DF_REG_DEF_COUNT (regno) != 1)
+        {
+          bool equal_p = true;
+          rtx_insn_list *list;
+
+          /* If we have already processed this pseudo and determined it
+         can not have an equivalence, then honor that decision.  */
+          if (reg_equiv[regno].no_equiv)
+        continue;
+
+          if (! note
            || rtx_varies_p (XEXP (note, 0), 0)
            || (reg_equiv[regno].replacement
                && ! rtx_equal_p (XEXP (note, 0),
-                    reg_equiv[regno].replacement))))
-        {
-          no_equiv (dest, set, NULL);
-          continue;
+                    reg_equiv[regno].replacement)))
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
+
+          list = reg_equiv[regno].init_insns;
+          for (; list; list = list->next ())
+        {
+          rtx note_tmp;
+          rtx_insn *insn_tmp;
+
+          insn_tmp = list->insn ();
+          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+          gcc_assert (note_tmp);
+          if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+            {
+              equal_p = false;
+              break;
+            }
+        }
+
+          if (! equal_p)
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
          }
+
        /* Record this insn as initializing this register.  */
        reg_equiv[regno].init_insns
          = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3514,10 +3550,9 @@ update_equiv_regs (void)
           a register used only in one basic block from a MEM.  If so, and the
           MEM remains unchanged for the life of the register, add a REG_EQUIV
           note.  */
-
        note = find_reg_note (insn, REG_EQUIV, NULL_RTX);

-      if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+      if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
            && MEM_P (SET_SRC (set))
            && validate_equiv_mem (insn, dest, SET_SRC (set)))
          note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
@@ -3547,7 +3582,7 @@ update_equiv_regs (void)

            reg_equiv[regno].replacement = x;
            reg_equiv[regno].src_p = &SET_SRC (set);
-          reg_equiv[regno].loop_depth = loop_depth;
+          reg_equiv[regno].loop_depth = (short) loop_depth;

            /* Don't mess with things live during setjmp.  */
            if (REG_LIVE_LENGTH (regno) >= 0 && optimize)
@@ -3677,7 +3712,7 @@ update_equiv_regs (void)
            rtx equiv_insn;

            if (! reg_equiv[regno].replace
-              || reg_equiv[regno].loop_depth < loop_depth
+              || reg_equiv[regno].loop_depth < (short) loop_depth
                /* There is no sense to move insns if live range
               shrinkage or register pressure-sensitive
               scheduling were done because it will not
Cheers,
Felix


On Tue, Sep 30, 2014 at 5:44 AM, Jeff Law <law@redhat.com> wrote:
On 09/27/14 08:48, Felix Yang wrote:

Thanks for the explaination.
I have changed the loop_depth into a short interger hoping that we can
save some memory :-)

Thanks.


Attached please find the updated patch. Bootstrapped and reg-tested on
x86_64-suse-linux.
Please do a final revew once the assignment is ready.

As for the new list walking interface, I choose the function
"no_equiv" and tried the "checked cast" way.
The bad news is that GCC failed to bootstrap with the following change:

Index: ira.c
===================================================================
--- ira.c (revision 215536)
+++ ira.c (working copy)
@@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
     void *data ATTRIBUTE_UNUSED)
   {
     int regno;
-  rtx list;
+  rtx_insn_list *list;

     if (!REG_P (reg))
       return;
     regno = REGNO (reg);
-  list = reg_equiv[regno].init_insns;
+  list = as_a <rtx_insn_list *> (reg_equiv[regno].init_insns);
     if (list == const0_rtx)
       return;
     reg_equiv[regno].init_insns = const0_rtx;
@@ -3258,9 +3258,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
       return;
     ira_reg_equiv[regno].defined_p = false;
     ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = list->next ())
       {
-      rtx insn = XEXP (list, 0);
+      rtx_insn *insn = list->insn ();
         remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
       }
   }

Yea.  I'm going to post a patch shortly to go ahead with this conversion.
There's a couple issues that come into play.

First const0_rtx is not an INSN, so we *really* don't want it in the INSN
field of an INSN_LIST.  That's probably the ICE you're seeing.

const0_rtx is being used to mark pseudos which we've already determined
can't have a valid equivalence.  So we just need a different marker. That
different marker must be embeddable in an INSN_LIST node.   The easiest is
just a NULL insn ;-)

The other tests for the const0_rtx marker in ira.c need relatively trivial
updating.  And in the end we don't need the checked cast at all ;-)



Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]