| Message ID | B5E67142681B53468FAF6B7C3135656244159813@hhmail02.hh.imgtec.org |
|---|---|
| State | New |
| Headers | show |
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 0000000..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 0000000..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
Robert Suchanek <Robert.Suchanek@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; >
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
Hi, > > Trim the extra trailing newline. > > OK to commit if you are happy with the comment. Committed as r224549. Regards, 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; +} + /* 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 0000000..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 0000000..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; +}