| Message ID | 871szkg2he.fsf@atmel.com |
|---|---|
| State | New |
| Headers | show |
Ping! Regards Senthil Senthil Kumar Selvaraj writes: > Bernd Schmidt writes: > >> On 09/16/2016 09:02 PM, Senthil Kumar Selvaraj wrote: >>> Does this make sense? I ran a reg test for the avr target with a >>> slightly older version of this patch, it did not show any regressions. >>> If this is the right fix, I'll make sure to run reg tests on x86_64 >>> after backporting to a gcc version where that target used reload. >> >> It's hard to say, and could have different effects on different targets. >> One thing though, at the very least the reg_class_size test would have >> to be adapted - the idea is to find the largest class, and there's a >> risk here of ending up with a large class that only has one valid register. > > Agreed - I've updated the patch to compute rclass sizes based on regno > availability i.e., only if in_hard_reg_set_p and HARD_REGNO_MODE_OK, and > then use the computed sizes when calculating best_size. > >> >> You'll also want to verify this against >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54814 > > Yes, this patch doesn't break the fix for PR54814. The change to > in_hard_reg_set_p was what fixed that, and that remains unmodified. > > Reg tested this on top of trunk@190252 with the in_hard_reg_set_p > backport. x86_64-pc-linux bootstrapped and regtested ok. avr showed > no regressions either. > > Ok for trunk? > > Regards > Senthil > > gcc/ChangeLog: > > 2016-10-13 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > * reload.c (find_valid_class_1): Allow regclass if atleast one > regno in class is ok. Compute and use rclass size based on > actually available regnos for mode in rclass. > > gcc/testsuite/ChangeLog: > > 2016-10-13 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > * gcc.target/avr/pr71627.c: New. > > > Index: gcc/reload.c > =================================================================== > --- gcc/reload.c (revision 240989) > +++ gcc/reload.c (working copy) > @@ -711,31 +711,36 @@ > enum reg_class best_class = NO_REGS; > unsigned int best_size = 0; > int cost; > + unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 }; > > for (rclass = 1; rclass < N_REG_CLASSES; rclass++) > { > - int bad = 0; > - for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++) > - { > - if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno) > - && !HARD_REGNO_MODE_OK (regno, mode)) > - bad = 1; > - } > - > - if (bad) > - continue; > + int atleast_one_regno_ok = 0; > > + for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) > + { > + if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)) > + { > + atleast_one_regno_ok = 1; > + if (HARD_REGNO_MODE_OK (regno, mode)) > + computed_rclass_sizes[rclass]++; > + } > + } > + > + if (!atleast_one_regno_ok) > + continue; > + > cost = register_move_cost (outer, (enum reg_class) rclass, dest_class); > > - if ((reg_class_size[rclass] > best_size > - && (best_cost < 0 || best_cost >= cost)) > - || best_cost > cost) > - { > - best_class = (enum reg_class) rclass; > - best_size = reg_class_size[rclass]; > - best_cost = register_move_cost (outer, (enum reg_class) rclass, > - dest_class); > - } > + if ((computed_rclass_sizes[rclass] > best_size > + && (best_cost < 0 || best_cost >= cost)) > + || best_cost > cost) > + { > + best_class = (enum reg_class) rclass; > + best_size = computed_rclass_sizes[rclass]; > + best_cost = register_move_cost (outer, (enum reg_class) rclass, > + dest_class); > + } > } > > gcc_assert (best_size != 0); > > Index: gcc/testsuite/gcc.target/avr/pr71627.c > =================================================================== > --- gcc/testsuite/gcc.target/avr/pr71627.c (nonexistent) > +++ gcc/testsuite/gcc.target/avr/pr71627.c (working copy) > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O1" } */ > + > + > +extern volatile __memx const long a, b, c, d, e, f; > +extern volatile long result; > + > +extern void vfunc (const char*, ...); > + > +void foo (void) > +{ > + result = a + b + c + d + e + f; > + vfunc ("text", a, b, c, d, e, f, result); > +}
On 10/13/2016 08:57 AM, Senthil Kumar Selvaraj wrote: > > 2016-10-13 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > * reload.c (find_valid_class_1): Allow regclass if atleast one > regno in class is ok. Compute and use rclass size based on > actually available regnos for mode in rclass. > > gcc/testsuite/ChangeLog: > > 2016-10-13 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > * gcc.target/avr/pr71627.c: New. > > > Index: gcc/reload.c > =================================================================== > --- gcc/reload.c (revision 240989) > +++ gcc/reload.c (working copy) > @@ -711,31 +711,36 @@ > enum reg_class best_class = NO_REGS; > unsigned int best_size = 0; > int cost; > + unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 }; As far as I can tell you're only accessing this as computed_rclass_size[rclass], i.e. with the current class in the loop. So I don't think you need the array at all, just a computed_size variable in the loop? > + for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) > + { > + if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)) > + { > + atleast_one_regno_ok = 1; > + if (HARD_REGNO_MODE_OK (regno, mode)) > + computed_rclass_sizes[rclass]++; > + } > + } Don't you want to also ensure HARD_REGNO_MODE_OK before claiming that atleast_one_regno_ok? Maybe I'm forgetting the motivation but this seems odd. If so, the variable becomes unnecessary, just check the computed size. Bernd
Bernd Schmidt writes: > On 10/13/2016 08:57 AM, Senthil Kumar Selvaraj wrote: >> >> 2016-10-13 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> >> >> * reload.c (find_valid_class_1): Allow regclass if atleast one >> regno in class is ok. Compute and use rclass size based on >> actually available regnos for mode in rclass. >> >> gcc/testsuite/ChangeLog: >> >> 2016-10-13 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> >> >> * gcc.target/avr/pr71627.c: New. >> >> >> Index: gcc/reload.c >> =================================================================== >> --- gcc/reload.c (revision 240989) >> +++ gcc/reload.c (working copy) >> @@ -711,31 +711,36 @@ >> enum reg_class best_class = NO_REGS; >> unsigned int best_size = 0; >> int cost; >> + unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 }; > > As far as I can tell you're only accessing this as > computed_rclass_size[rclass], i.e. with the current class in the loop. > So I don't think you need the array at all, just a computed_size > variable in the loop? Yes - I mechanically replaced the original array with the computed one. A variable would suffice. > >> + for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) >> + { >> + if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)) >> + { >> + atleast_one_regno_ok = 1; >> + if (HARD_REGNO_MODE_OK (regno, mode)) >> + computed_rclass_sizes[rclass]++; >> + } >> + } > > Don't you want to also ensure HARD_REGNO_MODE_OK before claiming that > atleast_one_regno_ok? Maybe I'm forgetting the motivation but this seems > odd. If so, the variable becomes unnecessary, just check the computed size. True again - the original intention was to prevent the best_xxx variables from getting set if no regno was in_hard_reg_set. Now the computed class size would be zero, so the variable is unnecessary. Will do both the changes and re-run the reg tests. Ok for trunk if the tests pass for x86_64-pc-linux and avr? Regards Senthil
On 10/18/2016 02:15 PM, Senthil Kumar Selvaraj wrote: > Will do both the changes and re-run the reg tests. Ok for trunk if the > tests pass for x86_64-pc-linux and avr? > Probably but let's see the patch first. Bernd
Index: gcc/reload.c =================================================================== --- gcc/reload.c (revision 240989) +++ gcc/reload.c (working copy) @@ -711,31 +711,36 @@ enum reg_class best_class = NO_REGS; unsigned int best_size = 0; int cost; + unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 }; for (rclass = 1; rclass < N_REG_CLASSES; rclass++) { - int bad = 0; - for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++) - { - if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno) - && !HARD_REGNO_MODE_OK (regno, mode)) - bad = 1; - } - - if (bad) - continue; + int atleast_one_regno_ok = 0; + for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) + { + if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)) + { + atleast_one_regno_ok = 1; + if (HARD_REGNO_MODE_OK (regno, mode)) + computed_rclass_sizes[rclass]++; + } + } + + if (!atleast_one_regno_ok) + continue; + cost = register_move_cost (outer, (enum reg_class) rclass, dest_class); - if ((reg_class_size[rclass] > best_size - && (best_cost < 0 || best_cost >= cost)) - || best_cost > cost) - { - best_class = (enum reg_class) rclass; - best_size = reg_class_size[rclass]; - best_cost = register_move_cost (outer, (enum reg_class) rclass, - dest_class); - } + if ((computed_rclass_sizes[rclass] > best_size + && (best_cost < 0 || best_cost >= cost)) + || best_cost > cost) + { + best_class = (enum reg_class) rclass; + best_size = computed_rclass_sizes[rclass]; + best_cost = register_move_cost (outer, (enum reg_class) rclass, + dest_class); + } } gcc_assert (best_size != 0); Index: gcc/testsuite/gcc.target/avr/pr71627.c =================================================================== --- gcc/testsuite/gcc.target/avr/pr71627.c (nonexistent) +++ gcc/testsuite/gcc.target/avr/pr71627.c (working copy) @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O1" } */ + + +extern volatile __memx const long a, b, c, d, e, f; +extern volatile long result; + +extern void vfunc (const char*, ...); + +void foo (void) +{ + result = a + b + c + d + e + f; + vfunc ("text", a, b, c, d, e, f, result); +}