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]

Fix g++.dg/tree-ssa/ivopts-1.C for cris-elf


With a recent correction to this test-case, it started failing
for cris-elf; the "offset:" non-matcher part to be specific.
Upon inspection, the correction appeared right; my testing is
done on a 64-bit host, so there'd be 4294967292 (unsigned -4),
not 0xfffffffc.  A gdb-session exposed a typo in the
cris_address_cost function; the wrong arm of a "plus" was
checked for being a register, yielding all (plus ...) addresses
the same cost.  Oops.  Anyway, with the patch below, a sane
address cost is generated.  It's enough to go from (reg + -4) to
(reg + reg) which might not be seen as an improvement (they have
the same cost for cris-elf), but on the other hand I think the
test is a bit faulty too for assuming that's a pessimization.
Still, the test exposed a bug, happy happy!

I noticed some random warts in tree-ssa-loop-ivopts.c while in
that gdb-session:

- For an address to be considered validly containing a multiple
of a register (mult reg N), that must be valid as an address on
its own.  This can't be very common; I believe most machines
having that capability requires at least a register to that
mult, like (plus (mult reg N) reg).  Something for the next
stage 1.

- If the address cost is 0, it is "upgraded" to 1 at line 2981,
(causing there to be no difference between costs 0 and 1).  I
see no reason for this; there's nothing saying 0 is odd for an
address cost.  In fact, (by visual inspection) the default value
for the address cost of a register is 0.  In the next stage 1, I
suggest this be changed to just add 1 to acost unilaterally if
it really needs to be > 0, but I'm also unsure why that code
takes the cost of a containing sequence; it seems to believe
calls to memory_address can cause RTX to be generated; that'd be
a bug, right?

This patch has a single operative change (s/tem1/tem2/) and a
few formatting fixes (indentation was wrong), an added assertion
to catch non-canonicalized rtl (assuming the canonical
representation is reg+int and reg+mem - which actually isn't
really defined for those sequences IIUC), and a missing
'#include "df.h"' which was omitted when dataflow changed to
df_regs_ever_live_p.  Besides the ivopts-related comment I
added, I also noticed a comment that seemed to refer to this
actual bug, but then again maybe something else, so I removed it
as confusing.

Anyway, this patch fixes the regression and introduces no other.
Committed.

	* config/cris/cris.c: Include df.h.
	(cris_address_cost): Add gcc_assert for canonicalization
	assumptions.  For PLUS with MULT or register, correct
	test for register in other arm.  Tweak comments.

Index: config/cris/cris.c
===================================================================
--- config/cris/cris.c	(revision 129704)
+++ config/cris/cris.c	(working copy)
@@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.  
 #include "target-def.h"
 #include "ggc.h"
 #include "optabs.h"
+#include "df.h"
 
 /* Usable when we have an amount to add or subtract, and want the
    optimal size of the insn.  */
@@ -1716,7 +1717,12 @@ cris_address_cost (rtx x)
   /* The metric to use for the cost-macros is unclear.
      The metric used here is (the number of cycles needed) / 2,
      where we consider equal a cycle for a word of code and a cycle to
-     read memory.  */
+     read memory.  FIXME: Adding "+ 1" to all values would avoid
+     returning 0, as tree-ssa-loop-ivopts.c as of r128272 "normalizes"
+     0 to 1, thereby giving equal costs to [rN + rM] and [rN].
+     Unfortunately(?) such a hack would expose other pessimizations,
+     at least with g++.dg/tree-ssa/ivopts-1.C, adding insns to the
+     loop there, without apparent reason.  */
 
   /* The cheapest addressing modes get 0, since nothing extra is needed.  */
   if (BASE_OR_AUTOINCR_P (x))
@@ -1738,34 +1744,34 @@ cris_address_cost (rtx x)
       rtx tem1 = XEXP (x, 0);
       rtx tem2 = XEXP (x, 1);
 
-    /* A BIAP is 2 extra bytes for the prefix insn, nothing more.  We
-       recognize the typical MULT which is always in tem1 because of
-       insn canonicalization.  */
-    if ((GET_CODE (tem1) == MULT && BIAP_INDEX_P (tem1))
-	|| REG_P (tem1))
-      return 2 / 2;
-
-    /* A BDAP (quick) is 2 extra bytes.  Any constant operand to the
-       PLUS is always found in tem2.  */
-    if (CONST_INT_P (tem2) && INTVAL (tem2) < 128 && INTVAL (tem2) >= -128)
-      return 2 / 2;
-
-    /* A BDAP -32768 .. 32767 is like BDAP quick, but with 2 extra
-       bytes.  */
-    if (CONST_INT_P (tem2) && CONST_OK_FOR_LETTER_P (INTVAL (tem2), 'L'))
-      return (2 + 2) / 2;
+      /* We'll "assume" canonical RTX.  */
+      gcc_assert (REG_P (tem1) || GET_CODE (tem1) == MULT);
 
-    /* A BDAP with some other constant is 2 bytes extra.  */
-    if (CONSTANT_P (tem2))
-      return (2 + 2 + 2) / 2;
+      /* A BIAP is 2 extra bytes for the prefix insn, nothing more.  We
+	 recognize the typical MULT which is always in tem1 because of
+	 insn canonicalization.  */
+      if ((GET_CODE (tem1) == MULT && BIAP_INDEX_P (tem1))
+	  || REG_P (tem2))
+	return 2 / 2;
+
+      /* A BDAP (quick) is 2 extra bytes.  Any constant operand to the
+	 PLUS is always found in tem2.  */
+      if (CONST_INT_P (tem2) && INTVAL (tem2) < 128 && INTVAL (tem2) >= -128)
+	return 2 / 2;
+
+      /* A BDAP -32768 .. 32767 is like BDAP quick, but with 2 extra
+	 bytes.  */
+      if (CONST_INT_P (tem2) && CONST_OK_FOR_LETTER_P (INTVAL (tem2), 'L'))
+	return (2 + 2) / 2;
+
+      /* A BDAP with some other constant is 2 bytes extra.  */
+      if (CONSTANT_P (tem2))
+	return (2 + 2 + 2) / 2;
 
-    /* BDAP with something indirect should have a higher cost than
-       BIAP with register.   FIXME: Should it cost like a MEM or more?  */
-    /* Don't need to check it, it's the only one left.
-       FIXME:  There was a REG test missing, perhaps there are others.
-       Think more.  */
-    return (2 + 2 + 2) / 2;
-  }
+      /* BDAP with something indirect should have a higher cost than
+	 BIAP with register.   FIXME: Should it cost like a MEM or more?  */
+      return (2 + 2 + 2) / 2;
+    }
 
   /* What else?  Return a high cost.  It matters only for valid
      addressing modes.  */

brgds, H-P


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