RE: [Patch MIPS] Enable TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS hook
Hi, Trim the extra trailing newline. OK to commit if you are happy with the comment. Committed as r224549. Regards, Robert
RE: [Patch MIPS] Enable TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS hook
Hi Matthew, /* LRA will allocate an FPR for an integer mode pseudo instead of spilling to memory if an FPR is present in the allocno class. It is rare that we actually need to place an integer mode value in an FPR so where possible limit the allocation to GR_REGS. This will slightly pessimize code that involves integer to/from float conversions as these will have to reload into FPRs in LRA. Such reloads are sometimes eliminated and sometimes only partially eliminated. We choose to take this penalty in order to eliminate usage of FPRs in code that does not use floating point data. This change has a similar effect to increasing the cost of FPR-GPR register moves for integer modes so that they are higher than the cost of memory but changing the allocno class is more reliable. This is also similar to forbidding integer mode values in FPRs entirely but this would lead to an inconsistency in the integer to/from float instructions that say integer mode values must be placed in FPRs. */ I'm keen to get the description of this right so please feel free to change it further if it isn't clear (or correct). This description is definitely more accurate. At first glance, I wasn't sure whether to include the bit about partial elimination since it caused by post reload passes but the introduction of reloads should be avoided in the first place. I don't know if this change will lead to classic reload being unusable for MIPS. I'm not worried about that but I think it is probably wise to remove classic reload support for MIPS now; we are dependent on LRA for several features already. Indeed. I think it's the right time to drop the classic reload and resolve LRA issues if they come up. Do you have any details on when we are left with suboptimal code for int-float conversions? I'd like to keep a record of them in this thread or in the comment so we know what is left to fix. I opened a bug report https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66204 and just realised that I had never CCed Vlad. Hopefully, this is the last remaining issue related to integers and floating-point registers. + if (INTEGRAL_MODE_P (PSEUDO_REGNO_MODE (regno)) allocno_class == ALL_REGS) +return GR_REGS; + return allocno_class; +} + Trim the extra trailing newline. OK to commit if you are happy with the comment. I'll update the comment, do a quick regression and commit it. Regards, Robert
RE: [Patch MIPS] Enable TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS hook
Robert Suchanek robert.sucha...@imgtec.com writes:
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index c3755f5..976f844 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -19415,6 +19415,21 @@ mips_lra_p (void)
{
return mips_lra_flag;
}
+
+/* Implement TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS. */
+
+static reg_class_t
+mips_ira_change_pseudo_allocno_class (int regno, reg_class_t
allocno_class)
+{
+ /* LRA will generate unnecessary reloads because the LRA's cost pass
finds
+ cheaper to move data to/from memory into FP regs rather than GP
regs.
+ By narrowing the class for allocnos to GR_REGS for integral modes
early,
+ we refrain from using FP regs until they are absolutely necessary.
*/
I'm not sure this comment is accurately describing the issue (or I have
misunderstood something). I thought this change is to counter LRA's
tendency to use an FPR as a spill instead of memory?
i.e.
/* LRA will allocate an FPR for an integer mode pseudo instead of spilling
to memory if an FPR is present in the allocno class. It is rare that
we actually need to place an integer mode value in an FPR so where
possible limit the allocation to GR_REGS. This will slightly pessimize
code that involves integer to/from float conversions as these will have
to reload into FPRs in LRA. Such reloads are sometimes eliminated and
sometimes only partially eliminated. We choose to take this penalty
in order to eliminate usage of FPRs in code that does not use floating
point data.
This change has a similar effect to increasing the cost of FPR-GPR
register moves for integer modes so that they are higher than the cost
of memory but changing the allocno class is more reliable.
This is also similar to forbidding integer mode values in FPRs entirely
but this would lead to an inconsistency in the integer to/from float
instructions that say integer mode values must be placed in FPRs. */
I'm keen to get the description of this right so please feel free to change
it further if it isn't clear (or correct).
I don't know if this change will lead to classic reload being unusable for
MIPS. I'm not worried about that but I think it is probably wise to
remove classic reload support for MIPS now; we are dependent on LRA for
several features already.
Do you have any details on when we are left with suboptimal code for
int-float conversions? I'd like to keep a record of them in this thread
or in the comment so we know what is left to fix.
+ if (INTEGRAL_MODE_P (PSEUDO_REGNO_MODE (regno)) allocno_class ==
ALL_REGS)
+return GR_REGS;
+ return allocno_class;
+}
+
Trim the extra trailing newline.
OK to commit if you are happy with the comment.
Thanks,
Matthew
/* Initialize the GCC target structure. */
#undef TARGET_ASM_ALIGNED_HI_OP
@@ -19671,6 +19686,8 @@ mips_lra_p (void)
#define TARGET_SPILL_CLASS mips_spill_class
#undef TARGET_LRA_P
#define TARGET_LRA_P mips_lra_p
+#undef TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
+#define TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
mips_ira_change_pseudo_allocno_class
struct gcc_target targetm = TARGET_INITIALIZER;
RE: [Patch MIPS] Enable TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS hook
Hi Matthew,
+
+/* Implement TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS. */
+
+static reg_class_t
+mips_ira_change_pseudo_allocno_class (int regno, reg_class_t
+allocno_class) {
+ if (FLOAT_MODE_P (PSEUDO_REGNO_MODE (regno)) || allocno_class !=
ALL_REGS)
+return allocno_class;
+ return GR_REGS;
+}
+
I'm concerned that this may not be the right condition but either way,
I think it is better to switch this around to have the special case
as the conditional. I found it difficult to understand what it is
doing even when I know the intent :-) A comment about the purpose seems
appropriate too here as it won't be obvious to someone new.
I tried to write a sensible comment and found the original change hard
to describe. I changed the condition to the special case and did some
experiments. The patch below is now more concise, better fits the purpose and
it
seems to have marginally better allocation too.
Aren't there some fixed point modes that should go in FPRs too? I guess
paired single (v2sf) doesn't need mentioning as it would never be
allowed in GR_REGS so pseudos of that mode would never get ALL_REGS,
is that correct? I.e. will we only see ALL_REGS if a particular
pseudo/mode truly can be placed in any register according to the
hard_regno_ok rules?
I think that with the patch below all concerns would be addressed since
the class narrowing would be constrained to integers rather than anything else.
Regards,
Robert
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index c3755f5..976f844 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -19415,6 +19415,21 @@ mips_lra_p (void)
{
return mips_lra_flag;
}
+
+/* Implement TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS. */
+
+static reg_class_t
+mips_ira_change_pseudo_allocno_class (int regno, reg_class_t allocno_class)
+{
+ /* LRA will generate unnecessary reloads because the LRA's cost pass finds
+ cheaper to move data to/from memory into FP regs rather than GP regs.
+ By narrowing the class for allocnos to GR_REGS for integral modes early,
+ we refrain from using FP regs until they are absolutely necessary. */
+ if (INTEGRAL_MODE_P (PSEUDO_REGNO_MODE (regno)) allocno_class == ALL_REGS)
+return GR_REGS;
+ return allocno_class;
+}
+
/* Initialize the GCC target structure. */
#undef TARGET_ASM_ALIGNED_HI_OP
@@ -19671,6 +19686,8 @@ mips_lra_p (void)
#define TARGET_SPILL_CLASS mips_spill_class
#undef TARGET_LRA_P
#define TARGET_LRA_P mips_lra_p
+#undef TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
+#define TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
mips_ira_change_pseudo_allocno_class
struct gcc_target targetm = TARGET_INITIALIZER;
RE: [Patch MIPS] Enable TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS hook
Hi Robert,
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index
c3755f5..3c8ac30 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -19415,6 +19415,17 @@ mips_lra_p (void) {
return mips_lra_flag;
}
+
+/* Implement TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS. */
+
+static reg_class_t
+mips_ira_change_pseudo_allocno_class (int regno, reg_class_t
+allocno_class) {
+ if (FLOAT_MODE_P (PSEUDO_REGNO_MODE (regno)) || allocno_class !=
ALL_REGS)
+return allocno_class;
+ return GR_REGS;
+}
+
I'm concerned that this may not be the right condition but either way,
I think it is better to switch this around to have the special case
as the conditional. I found it difficult to understand what it is
doing even when I know the intent :-) A comment about the purpose seems
appropriate too here as it won't be obvious to someone new.
Aren't there some fixed point modes that should go in FPRs too? I guess
paired single (v2sf) doesn't need mentioning as it would never be
allowed in GR_REGS so pseudos of that mode would never get ALL_REGS,
is that correct? I.e. will we only see ALL_REGS if a particular
pseudo/mode truly can be placed in any register according to the
hard_regno_ok rules?
Thanks,
Matthew
/* Initialize the GCC target structure. */ #undef
TARGET_ASM_ALIGNED_HI_OP @@ -19671,6 +19682,8 @@ mips_lra_p (void)
#define TARGET_SPILL_CLASS mips_spill_class #undef TARGET_LRA_P
#define TARGET_LRA_P mips_lra_p
+#undef TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
+#define TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
+mips_ira_change_pseudo_allocno_class
struct gcc_target targetm = TARGET_INITIALIZER;
diff --git a/gcc/testsuite/gcc.target/mips/pr65862-1.c
b/gcc/testsuite/gcc.target/mips/pr65862-1.c
new file mode 100644
index 000..0c00092
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr65862-1.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-skip-if code quality test { *-*-* } { -O0 } { } } */
+/* { dg-final { scan-assembler-not \\\$f\[0-9\]+ } } */ int a, c; int
+*b, *d; void fn1(int p1, int *p2(void *, void *), void *p3(void *, void
+*, int)) {
+ int n = c;
+ for (;;) {
+a = 1;
+for (; a n;) {
+ *d = p1 p2(0, (int *) ((long)p1 + 1));
+ p3(0, b + p1, 0);
+}
+ }
+}
diff --git a/gcc/testsuite/gcc.target/mips/pr65862-2.c
b/gcc/testsuite/gcc.target/mips/pr65862-2.c
new file mode 100644
index 000..c6a2641
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr65862-2.c
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-skip-if code quality test { *-*-* } { -O0 } { } } */
+/* { dg-final { scan-assembler-not \\\$f\[0-9\]+ } } */ int a, b, d,
+e, j, k, n, o; unsigned c, h, i, l, m, p; int *f; int *g; int fn1(int
+p1) { return p1 - a; }
+
+int fn2() {
+ b = b + 1 - a;
+ e = 1 + o + 1518500249;
+ d = d + n;
+ c = (int)c + g[0];
+ b = b + m + 1;
+ d = d + p + 1518500249;
+ d = d + k - 1;
+ c = fn1(c + j + 1518500249);
+ e = fn1(e + i + 1);
+ d = d + h + 1859775393 - a;
+ c = fn1(c + (d ^ 1 ^ b) + g[1] + 1);
+ b = fn1(b + m + 3);
+ d = fn1(d + l + 1);
+ b = b + (c ^ 1) + p + 1;
+ e = fn1(e + (b ^ c ^ d) + n + 1);
+ d = o;
+ b = 0;
+ e = e + k + 1859775393;
+ f[0] = e;
+ return a;
+}
--
2.2.2
