diff mbox

[RFA,PR,rtl-optimization/70263] Fix creation of new REG_EQUIV notes

Message ID 56EC5391.70807@redhat.com
State New
Headers show

Commit Message

Jeff Law March 18, 2016, 7:14 p.m. UTC 
On 03/17/2016 12:23 PM, Bernd Schmidt wrote:
> On 03/17/2016 06:37 PM, Jeff Law wrote:
>> +  bitmap seen_insns;
>>
>> +  seen_insns = BITMAP_ALLOC (NULL);
>
> You could save an allocation here by making this a bitmap_head and using
> bitmap_initialize.
Done.

>
>> +      bitmap_set_bit (seen_insns, INSN_UID (insn));
>> +
>>        if (! INSN_P (insn))
>>      continue;
>>
>> @@ -3646,7 +3656,8 @@ update_equiv_regs (void)
>>        && ! find_reg_note (XEXP (reg_equiv[regno].init_insns, 0),
>>                    REG_EQUIV, NULL_RTX)
>>        && ! contains_replace_regs (XEXP (dest, 0))
>> -      && ! pdx_subregs[regno])
>> +      && ! pdx_subregs[regno]
>> +      && ! bitmap_bit_p (seen_insns, INSN_UID (insn)))
>
> This looks odd to me. Isn't this condition always false? Did you want to
> test the init_insn?
Right, we need to test that we found INIT_INSN prior to handling INSN. 
The test needs to go into the next conditional, after we initialize 
INIT_INSN.

I double-checked, we'll only set REG_BASIC_BLOCK if the set and all 
users are in the same block.  That was my recollection, but after the 
major goof in V1 of the patch, I wanted to be sure.

I also added a blurb to the dump file when we create these equivalences 
and included a test to verify the code fires.  I verified it fired on 
x86 and x86-64.  It may or may not fire on other targets, so I left the 
test in the i386 specific subdirectory.


Bootstrapped and regression tested on x86_64-linux-gnu.  And unlike the 
last version, it doesn't totally disable the note creation (as verified 
by the new test ;-)

OK for the trunk?

Thanks,
Jeff

Comments

Bernd Schmidt March 18, 2016, 7:16 p.m. UTC | #1 
On 03/18/2016 08:14 PM, Jeff Law wrote:
> I also added a blurb to the dump file when we create these equivalences
> and included a test to verify the code fires.  I verified it fired on
> x86 and x86-64.  It may or may not fire on other targets, so I left the
> test in the i386 specific subdirectory.

This is the sort of thing I'd want to do with rtl unit tests.
>
> Bootstrapped and regression tested on x86_64-linux-gnu.  And unlike the
> last version, it doesn't totally disable the note creation (as verified
> by the new test ;-)
>
> OK for the trunk?

Ok.


Bernd
Jeff Law March 18, 2016, 7:20 p.m. UTC | #2 
On 03/18/2016 01:16 PM, Bernd Schmidt wrote:
> On 03/18/2016 08:14 PM, Jeff Law wrote:
>> I also added a blurb to the dump file when we create these equivalences
>> and included a test to verify the code fires.  I verified it fired on
>> x86 and x86-64.  It may or may not fire on other targets, so I left the
>> test in the i386 specific subdirectory.
>
> This is the sort of thing I'd want to do with rtl unit tests.
Yea.  Along the same lines, my patch for the coalescing problem 
introduces a new bitmap function that I'd like to cover with some unit 
tests.  I'm sure we're going to find oodles of these things as we 
continue development and ponder how to better test things than scanning 
dump files.

Jeff
David Malcolm March 18, 2016, 8:05 p.m. UTC | #3 
On Fri, 2016-03-18 at 13:20 -0600, Jeff Law wrote:
> On 03/18/2016 01:16 PM, Bernd Schmidt wrote:
> > On 03/18/2016 08:14 PM, Jeff Law wrote:
> > > I also added a blurb to the dump file when we create these
> > > equivalences
> > > and included a test to verify the code fires.  I verified it
> > > fired on
> > > x86 and x86-64.  It may or may not fire on other targets, so I
> > > left the
> > > test in the i386 specific subdirectory.
> > 
> > This is the sort of thing I'd want to do with rtl unit tests.

Would you like an RTL frontend?  (and something like a 
 gcc/testsuite/rtl.dg/ )

> Yea.  Along the same lines, my patch for the coalescing problem 
> introduces a new bitmap function that I'd like to cover with some
> unit 
> tests.  

Presumably the -fself-test idea could help here?

> I'm sure we're going to find oodles of these things as we 
> continue development and ponder how to better test things than
> scanning 
> dump files.
Jakub Jelinek March 18, 2016, 8:07 p.m. UTC | #4 
On Fri, Mar 18, 2016 at 04:05:08PM -0400, David Malcolm wrote:
> On Fri, 2016-03-18 at 13:20 -0600, Jeff Law wrote:
> > On 03/18/2016 01:16 PM, Bernd Schmidt wrote:
> > > On 03/18/2016 08:14 PM, Jeff Law wrote:
> > > > I also added a blurb to the dump file when we create these
> > > > equivalences
> > > > and included a test to verify the code fires.  I verified it
> > > > fired on
> > > > x86 and x86-64.  It may or may not fire on other targets, so I
> > > > left the
> > > > test in the i386 specific subdirectory.
> > > 
> > > This is the sort of thing I'd want to do with rtl unit tests.
> 
> Would you like an RTL frontend?  (and something like a 
>  gcc/testsuite/rtl.dg/ )

That really shouldn't be hard, we already have RTL readers and RTL writers,
of course e.g. stuff where RTL refers to trees will be harder (or we could
just not fill it in).

	Jakub
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index b7711b8..6b57495 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,12 @@ 
+2016-03-18  Jeff Law  <law@redhat.com>
+
+	PR rtl-optimization/70263
+	* ira.c (memref_used_between_p): Assert we found END in the insn chain.
+	(update_equiv_regs): When trying to move a store to after the insn
+	that sets the source of the store, make sure the store occurs after
+	the insn that sets the source of the store.  When successful note
+	the REG_EQUIV note created in the dump file.
+
 2016-03-17  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR driver/70192
diff --git a/gcc/ira.c b/gcc/ira.c
index 062b8a4..c12318a 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -3225,13 +3225,18 @@  memref_referenced_p (rtx memref, rtx x)
 }
 
 /* TRUE if some insn in the range (START, END] references a memory location
-   that would be affected by a store to MEMREF.  */
+   that would be affected by a store to MEMREF.
+
+   Callers should not call this routine if START is after END in the
+   RTL chain.  */
+
 static int
 memref_used_between_p (rtx memref, rtx_insn *start, rtx_insn *end)
 {
   rtx_insn *insn;
 
-  for (insn = NEXT_INSN (start); insn != NEXT_INSN (end);
+  for (insn = NEXT_INSN (start);
+       insn && insn != NEXT_INSN (end);
        insn = NEXT_INSN (insn))
     {
       if (!NONDEBUG_INSN_P (insn))
@@ -3245,6 +3250,7 @@  memref_used_between_p (rtx memref, rtx_insn *start, rtx_insn *end)
 	return 1;
     }
 
+  gcc_assert (insn == NEXT_INSN (end));
   return 0;
 }
 
@@ -3337,6 +3343,7 @@  update_equiv_regs (void)
   int loop_depth;
   bitmap cleared_regs;
   bool *pdx_subregs;
+  bitmap_head seen_insns;
 
   /* Use pdx_subregs to show whether a reg is used in a paradoxical
      subreg.  */
@@ -3606,11 +3613,14 @@  update_equiv_regs (void)
   /* A second pass, to gather additional equivalences with memory.  This needs
      to be done after we know which registers we are going to replace.  */
 
+  bitmap_initialize (&seen_insns, NULL);
   for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
     {
       rtx set, src, dest;
       unsigned regno;
 
+      bitmap_set_bit (&seen_insns, INSN_UID (insn));
+
       if (! INSN_P (insn))
 	continue;
 
@@ -3651,6 +3661,7 @@  update_equiv_regs (void)
 	  rtx_insn *init_insn =
 	    as_a <rtx_insn *> (XEXP (reg_equiv[regno].init_insns, 0));
 	  if (validate_equiv_mem (init_insn, src, dest)
+	      && bitmap_bit_p (&seen_insns, INSN_UID (init_insn))
 	      && ! memref_used_between_p (dest, init_insn, insn)
 	      /* Attaching a REG_EQUIV note will fail if INIT_INSN has
 		 multiple sets.  */
@@ -3661,9 +3672,15 @@  update_equiv_regs (void)
 	      ira_reg_equiv[regno].init_insns
 		= gen_rtx_INSN_LIST (VOIDmode, insn, NULL_RTX);
 	      df_notes_rescan (init_insn);
+	      if (dump_file)
+		fprintf (dump_file,
+			 "Adding REG_EQUIV to insn %d for source of insn %d\n",
+			 INSN_UID (init_insn),
+			 INSN_UID (insn));
 	    }
 	}
     }
+  bitmap_clear (&seen_insns);
 
   cleared_regs = BITMAP_ALLOC (NULL);
   /* Now scan all regs killed in an insn to see if any of them are
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 7fe7295..336de6a 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2016-03-18  Jeff Law  <law@redhat.com>
+
+	PR rtl-optimization/70263
+	* gcc.c-torture/compile/pr70263-1.c: New test.
+	* gcc.target/i386/pr70263-2.c: New test.
+
 2016-03-17  Jakub Jelinek  <jakub@redhat.com>
 
 	PR c++/70144
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr70263-1.c b/gcc/testsuite/gcc.c-torture/compile/pr70263-1.c
new file mode 100644
index 0000000..d4bf280
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr70263-1.c
@@ -0,0 +1,11 @@ 
+int a[91];
+int b, c;
+void fn1() {
+  int n, m;
+  do {
+    a[c--];
+    a[--c] = m;
+    a[--m] = b;
+  } while (n);
+}
+
diff --git a/gcc/testsuite/gcc.target/i386/pr70263-2.c b/gcc/testsuite/gcc.target/i386/pr70263-2.c
new file mode 100644
index 0000000..18ebbf0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr70263-2.c
@@ -0,0 +1,23 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-ira" } */
+
+/* { dg-final { scan-rtl-dump "Adding REG_EQUIV to insn \[0-9\]+ for source of insn \[0-9\]+" "ira" } } */
+
+typedef float XFtype __attribute__ ((mode (XF)));
+typedef _Complex float XCtype __attribute__ ((mode (XC)));
+XCtype
+__mulxc3 (XFtype a, XFtype b, XFtype c, XFtype d)
+{
+  XFtype ac, bd, ad, bc, x, y;
+  ac = a * c;
+__asm__ ("": "=m" (ac):"m" (ac));
+  if (x != x)
+    {
+      _Bool recalc = 0;
+      if (((!(!(((ac) - (ac)) != ((ac) - (ac)))))))
+	recalc = 1;
+      if (recalc)
+	x = __builtin_huge_vall () * (a * c - b * d);
+    }
+  return x;
+}