Patchwork patch to fix PR55433

login
register
mail settings
Submitter Vladimir Makarov
Date Jan. 18, 2013, 6:25 p.m.
Message ID <50F993AD.5000509@redhat.com>
Download mbox | patch
Permalink /patch/213715/
State New
Headers show

Comments

Vladimir Makarov - Jan. 18, 2013, 6:25 p.m.
The following patch fixes

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55433

LRA went into infinite reload loop when secondary memory was needed.

To fix the problem, two changes are done:
    o a typo was fixed in inheritance which prevented rejecting 
inheritances requiring secondary memory reloads
    o still a spilled pseudo was reassigned to a register in LRA 
resulting in secondary memory reloads.  it is desirable to prevent it 
too but it is hard to do at this stage.
    o in any case we should have prevented infinite reload loop and it 
was achieved by reusing an original insn for secondary memory reloads.  
It is useful in case of the PR as the original insn is not a simple move 
(it contains also a pseudo clobber).

The last change is not trivial.  I worked on it for a few day.  I guess 
there is a tiny possibility that it breaks something but I myself did 
not any problems with the change.

The patch was successfully bootstrapped and tested on x86/x86-64.

Committed as rev. 195302.

2013-01-18  Vladimir Makarov  <vmakarov@redhat.com>

         PR target/55433
         * lra-constraints.c (curr_insn_transform): Reuse original insn for
         secondary memory move.
         (inherit_reload_reg): Use rclass instead of cl for
         check_secondary_memory_needed_p.

2013-01-18  Vladimir Makarov  <vmakarov@redhat.com>

         PR target/55433
         * gcc.target/i386/pr55433.c: New.
Steven Bosscher - Jan. 18, 2013, 6:34 p.m.
On Fri, Jan 18, 2013 at 7:25 PM, Vladimir Makarov wrote:
>
>         PR target/55433
>         * lra-constraints.c (curr_insn_transform): Reuse original insn for
>         secondary memory move.
>         (inherit_reload_reg): Use rclass instead of cl for
>         check_secondary_memory_needed_p.

Can you put this on the LRA branch also, please?

Thanks,

Ciao!
Steven
Vladimir Makarov - Jan. 18, 2013, 11:11 p.m.
On 13-01-18 1:34 PM, Steven Bosscher wrote:
> On Fri, Jan 18, 2013 at 7:25 PM, Vladimir Makarov wrote:
>>          PR target/55433
>>          * lra-constraints.c (curr_insn_transform): Reuse original insn for
>>          secondary memory move.
>>          (inherit_reload_reg): Use rclass instead of cl for
>>          check_secondary_memory_needed_p.
> Can you put this on the LRA branch also, please?
>
>
I merged trunk with this patch into LRA branch instead.

Patch

Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 195301)
+++ lra-constraints.c	(working copy)
@@ -2791,30 +2791,42 @@  curr_insn_transform (void)
 
   if (use_sec_mem_p)
     {
-      rtx new_reg, set, src, dest;
-      enum machine_mode sec_mode;
+      rtx new_reg, src, dest, rld, rld_subst;
+      enum machine_mode sec_mode, rld_mode;
 
       lra_assert (sec_mem_p);
-      set = single_set (curr_insn);
-      lra_assert (set != NULL_RTX && ! side_effects_p (set));
-      dest = SET_DEST (set);
-      src = SET_SRC (set);
+      lra_assert (curr_static_id->operand[0].type == OP_OUT
+		  && curr_static_id->operand[1].type == OP_IN);
+      dest = *curr_id->operand_loc[0];
+      src = *curr_id->operand_loc[1];
+      rld = (GET_MODE_SIZE (GET_MODE (dest)) <= GET_MODE_SIZE (GET_MODE (src))
+	     ? dest : src);
+      rld_mode = GET_MODE (rld);
 #ifdef SECONDARY_MEMORY_NEEDED_MODE
-      sec_mode = SECONDARY_MEMORY_NEEDED_MODE (GET_MODE (src));
+      sec_mode = SECONDARY_MEMORY_NEEDED_MODE (rld_mode);
 #else
-      sec_mode = GET_MODE (src);
+      sec_mode = rld_mode;
 #endif
       new_reg = lra_create_new_reg (sec_mode, NULL_RTX,
 				    NO_REGS, "secondary");
       /* If the mode is changed, it should be wider.  */
-      lra_assert (GET_MODE_SIZE (GET_MODE (new_reg))
-		  >= GET_MODE_SIZE (GET_MODE (src)));
-      after = emit_spill_move (false, new_reg, dest);
-      lra_process_new_insns (curr_insn, NULL_RTX, after,
-			     "Inserting the sec. move");
-      before = emit_spill_move (true, new_reg, src);
-      lra_process_new_insns (curr_insn, before, NULL_RTX, "Changing on");
-      lra_set_insn_deleted (curr_insn);
+      lra_assert (GET_MODE_SIZE (sec_mode) >= GET_MODE_SIZE (rld_mode));
+      rld_subst = (sec_mode == rld_mode ? new_reg : gen_lowpart_SUBREG (rld_mode, new_reg));
+      if (dest == rld)
+	{
+	  *curr_id->operand_loc[0] = rld_subst;
+	  after = emit_spill_move (false, new_reg, dest);
+	  lra_process_new_insns (curr_insn, NULL_RTX, after,
+				 "Inserting the sec. move");
+	}
+      else
+	{
+	  *curr_id->operand_loc[1] = rld_subst;
+	  before = emit_spill_move (true, new_reg, src);
+	  lra_process_new_insns (curr_insn, before, NULL_RTX,
+				 "Inserting the sec. move");
+	}
+      lra_update_insn_regno_info (curr_insn);
       return true;
     }
 #endif
@@ -3801,7 +3813,7 @@  inherit_reload_reg (bool def_p, int orig
 
       rclass = cl;
     }
-  if (check_secondary_memory_needed_p (cl, next_usage_insns))
+  if (check_secondary_memory_needed_p (rclass, next_usage_insns))
     {
       /* Reject inheritance resulting in secondary memory moves.
 	 Otherwise, there is a danger in LRA cycling.  Also such
@@ -3820,7 +3832,7 @@  inherit_reload_reg (bool def_p, int orig
 		   "    Rejecting inheritance for insn %d(%s)<-%d(%s) "
 		   "as secondary mem is needed\n",
 		   REGNO (dest), reg_class_names[get_reg_class (REGNO (dest))],
-		   original_regno, reg_class_names[cl]);
+		   original_regno, reg_class_names[rclass]);
 	  fprintf (lra_dump_file,
 		   "    >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>\n");
 	}
Index: testsuite/gcc.target/i386/pr55433.c
===================================================================
--- testsuite/gcc.target/i386/pr55433.c	(revision 0)
+++ testsuite/gcc.target/i386/pr55433.c	(working copy)
@@ -0,0 +1,12 @@ 
+/* { dg-do compile {target { *-*-darwin* } } } */
+/* { dg-options "-O1 -m32" } */
+
+typedef unsigned long long tick_t;
+extern int foo(void);
+extern tick_t tick(void);
+double test(void) {
+  struct { tick_t ticks; } st;
+  st.ticks = tick();
+  foo();
+  return (double)st.ticks;
+}