| Message ID | 4D007252.7060102@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote: > Hi, > this patch fixes the ICE in PR44557. It now occurs on trunk only under > quite specific option conditions, but debugging this PR leaded to quite > obvious Thumb-1 fixes. > > Also added a simplified testcase, derived from the one on bugzilla. > Tested without regressions, okay to commit to trunk? > > Thanks, > Chung-Lin > > 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com> > > PR target/44557 > * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to > Thumb-1 return LO_REGS case. > * config/arm/arm.md (reload_inhi): Change scratch constraint > from 'r' to 'l'. > > testsuite/ > * gcc.target/arm/pr44557.c: New. > Changing the scratch constraint to 'l' is going to pessimize thumb2 code reload generation which can quite reasonably take an 'r' class here. I'm not sure there's an easy way to fix this without going the whole hog and getting rid of reload_inhi entirely (which would be a good thing, TM) and replacing it with the new reload infrastructure that Joern wrote a few years back. Before approving this I want to be sure that it doesn't cause major problems on Thumb-2. What testing have you done for that configuration? R. PS: Maybe another way to fix this would be to introduce a new register class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on Thumb-1 only.
On 2010/12/21 02:03, Richard Earnshaw wrote: > > On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote: >> Hi, >> this patch fixes the ICE in PR44557. It now occurs on trunk only under >> quite specific option conditions, but debugging this PR leaded to quite >> obvious Thumb-1 fixes. >> >> Also added a simplified testcase, derived from the one on bugzilla. >> Tested without regressions, okay to commit to trunk? >> >> Thanks, >> Chung-Lin >> >> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com> >> >> PR target/44557 >> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to >> Thumb-1 return LO_REGS case. >> * config/arm/arm.md (reload_inhi): Change scratch constraint >> from 'r' to 'l'. >> >> testsuite/ >> * gcc.target/arm/pr44557.c: New. >> > > Changing the scratch constraint to 'l' is going to pessimize thumb2 code > reload generation which can quite reasonably take an 'r' class here. > > I'm not sure there's an easy way to fix this without going the whole hog > and getting rid of reload_inhi entirely (which would be a good thing, > TM) and replacing it with the new reload infrastructure that Joern wrote > a few years back. > > Before approving this I want to be sure that it doesn't cause major > problems on Thumb-2. What testing have you done for that configuration? > > R. > > PS: Maybe another way to fix this would be to introduce a new register > class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on > Thumb-1 only. > Hi Richard, My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what the ICE was about. I later tried another test run with Thumb-2 as you suggested, to be sure, and saw no regressions too. I've then been looking into the secondary reload logic. The ARM backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns NO_REGS under TARGET_32BIT for integer modes (no secondary reloads needed). Looking at the default_secondary_reload() function, this means that the reload_in/outhi patterns are never used for Thumb-2. So this patch should only affect Thumb-1, and the Thumb-2 considerations should be unneeded. As for the issue of replacing all of this with the more modern TARGET_SECONDARY_RELOAD machinery, that does seem attractive. Do you think such a patch can get into 4.6? Thanks, Chung-Lin ps. side question, the arm_reload_in/out_hi() functions does quite some tricks to do halfword-load/storing. This should be for pre-ARMv4 only right? (before the ldrh/strh instructions became available)
On 2011/1/1 19:21, Chung-Lin Tang wrote: > On 2010/12/21 02:03, Richard Earnshaw wrote: >> >> On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote: >>> Hi, >>> this patch fixes the ICE in PR44557. It now occurs on trunk only under >>> quite specific option conditions, but debugging this PR leaded to quite >>> obvious Thumb-1 fixes. >>> >>> Also added a simplified testcase, derived from the one on bugzilla. >>> Tested without regressions, okay to commit to trunk? >>> >>> Thanks, >>> Chung-Lin >>> >>> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com> >>> >>> PR target/44557 >>> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to >>> Thumb-1 return LO_REGS case. >>> * config/arm/arm.md (reload_inhi): Change scratch constraint >>> from 'r' to 'l'. >>> >>> testsuite/ >>> * gcc.target/arm/pr44557.c: New. >>> >> >> Changing the scratch constraint to 'l' is going to pessimize thumb2 code >> reload generation which can quite reasonably take an 'r' class here. >> >> I'm not sure there's an easy way to fix this without going the whole hog >> and getting rid of reload_inhi entirely (which would be a good thing, >> TM) and replacing it with the new reload infrastructure that Joern wrote >> a few years back. >> >> Before approving this I want to be sure that it doesn't cause major >> problems on Thumb-2. What testing have you done for that configuration? >> >> R. >> >> PS: Maybe another way to fix this would be to introduce a new register >> class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on >> Thumb-1 only. >> > > Hi Richard, > > My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what > the ICE was about. I later tried another test run with Thumb-2 as you > suggested, to be sure, and saw no regressions too. > > I've then been looking into the secondary reload logic. The ARM > backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns > NO_REGS under TARGET_32BIT for integer modes (no secondary reloads > needed). Looking at the default_secondary_reload() function, this means > that the reload_in/outhi patterns are never used for Thumb-2. > > So this patch should only affect Thumb-1, and the Thumb-2 considerations > should be unneeded. Hi Richard, pinging this patch. I think I've explained the effects of this fix, and it should be Thumb-1 only. Is this patch okay? Thanks, Chung-Lin
Ping. On 2011/1/26 11:07 AM, Chung-Lin Tang wrote: > On 2011/1/1 19:21, Chung-Lin Tang wrote: >> On 2010/12/21 02:03, Richard Earnshaw wrote: >>> >>> On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote: >>>> Hi, >>>> this patch fixes the ICE in PR44557. It now occurs on trunk only under >>>> quite specific option conditions, but debugging this PR leaded to quite >>>> obvious Thumb-1 fixes. >>>> >>>> Also added a simplified testcase, derived from the one on bugzilla. >>>> Tested without regressions, okay to commit to trunk? >>>> >>>> Thanks, >>>> Chung-Lin >>>> >>>> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com> >>>> >>>> PR target/44557 >>>> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to >>>> Thumb-1 return LO_REGS case. >>>> * config/arm/arm.md (reload_inhi): Change scratch constraint >>>> from 'r' to 'l'. >>>> >>>> testsuite/ >>>> * gcc.target/arm/pr44557.c: New. >>>> >>> >>> Changing the scratch constraint to 'l' is going to pessimize thumb2 code >>> reload generation which can quite reasonably take an 'r' class here. >>> >>> I'm not sure there's an easy way to fix this without going the whole hog >>> and getting rid of reload_inhi entirely (which would be a good thing, >>> TM) and replacing it with the new reload infrastructure that Joern wrote >>> a few years back. >>> >>> Before approving this I want to be sure that it doesn't cause major >>> problems on Thumb-2. What testing have you done for that configuration? >>> >>> R. >>> >>> PS: Maybe another way to fix this would be to introduce a new register >>> class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on >>> Thumb-1 only. >>> >> >> Hi Richard, >> >> My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what >> the ICE was about. I later tried another test run with Thumb-2 as you >> suggested, to be sure, and saw no regressions too. >> >> I've then been looking into the secondary reload logic. The ARM >> backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns >> NO_REGS under TARGET_32BIT for integer modes (no secondary reloads >> needed). Looking at the default_secondary_reload() function, this means >> that the reload_in/outhi patterns are never used for Thumb-2. >> >> So this patch should only affect Thumb-1, and the Thumb-2 considerations >> should be unneeded. > > Hi Richard, pinging this patch. > I think I've explained the effects of this fix, and it should be Thumb-1 > only. Is this patch okay? > > Thanks, > Chung-Lin
Ping. On 2011/3/7 10:11 PM, Chung-Lin Tang wrote: > Ping. > > On 2011/1/26 11:07 AM, Chung-Lin Tang wrote: >> On 2011/1/1 19:21, Chung-Lin Tang wrote: >>> On 2010/12/21 02:03, Richard Earnshaw wrote: >>>> >>>> On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote: >>>>> Hi, >>>>> this patch fixes the ICE in PR44557. It now occurs on trunk only under >>>>> quite specific option conditions, but debugging this PR leaded to quite >>>>> obvious Thumb-1 fixes. >>>>> >>>>> Also added a simplified testcase, derived from the one on bugzilla. >>>>> Tested without regressions, okay to commit to trunk? >>>>> >>>>> Thanks, >>>>> Chung-Lin >>>>> >>>>> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com> >>>>> >>>>> PR target/44557 >>>>> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to >>>>> Thumb-1 return LO_REGS case. >>>>> * config/arm/arm.md (reload_inhi): Change scratch constraint >>>>> from 'r' to 'l'. >>>>> >>>>> testsuite/ >>>>> * gcc.target/arm/pr44557.c: New. >>>>> >>>> >>>> Changing the scratch constraint to 'l' is going to pessimize thumb2 code >>>> reload generation which can quite reasonably take an 'r' class here. >>>> >>>> I'm not sure there's an easy way to fix this without going the whole hog >>>> and getting rid of reload_inhi entirely (which would be a good thing, >>>> TM) and replacing it with the new reload infrastructure that Joern wrote >>>> a few years back. >>>> >>>> Before approving this I want to be sure that it doesn't cause major >>>> problems on Thumb-2. What testing have you done for that configuration? >>>> >>>> R. >>>> >>>> PS: Maybe another way to fix this would be to introduce a new register >>>> class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on >>>> Thumb-1 only. >>>> >>> >>> Hi Richard, >>> >>> My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what >>> the ICE was about. I later tried another test run with Thumb-2 as you >>> suggested, to be sure, and saw no regressions too. >>> >>> I've then been looking into the secondary reload logic. The ARM >>> backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns >>> NO_REGS under TARGET_32BIT for integer modes (no secondary reloads >>> needed). Looking at the default_secondary_reload() function, this means >>> that the reload_in/outhi patterns are never used for Thumb-2. >>> >>> So this patch should only affect Thumb-1, and the Thumb-2 considerations >>> should be unneeded. >> >> Hi Richard, pinging this patch. >> I think I've explained the effects of this fix, and it should be Thumb-1 >> only. Is this patch okay? >> >> Thanks, >> Chung-Lin >
Index: gcc/config/arm/arm.h =================================================================== --- gcc/config/arm/arm.h (revision 167626) +++ gcc/config/arm/arm.h (working copy) @@ -1207,6 +1207,7 @@ (TARGET_32BIT ? (CLASS) : \ ((CLASS) == GENERAL_REGS || (CLASS) == HI_REGS \ || (CLASS) == NO_REGS || (CLASS) == STACK_REG \ + || (CLASS) == CORE_REGS \ ? LO_REGS : (CLASS))) /* Must leave BASE_REGS reloads alone */ Index: gcc/config/arm/arm.md =================================================================== --- gcc/config/arm/arm.md (revision 167626) +++ gcc/config/arm/arm.md (working copy) @@ -5850,7 +5850,7 @@ (define_expand "reload_inhi" [(parallel [(match_operand:HI 0 "s_register_operand" "=r") (match_operand:HI 1 "arm_reload_memory_operand" "o") - (match_operand:DI 2 "s_register_operand" "=&r")])] + (match_operand:DI 2 "s_register_operand" "=&l")])] "TARGET_EITHER" " if (TARGET_ARM) Index: gcc/testsuite/gcc.target/arm/pr44557.c =================================================================== --- gcc/testsuite/gcc.target/arm/pr44557.c (revision 0) +++ gcc/testsuite/gcc.target/arm/pr44557.c (revision 0) @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-mthumb -O1 -march=armv5te -fno-omit-frame-pointer -fno-forward-propagate" } */ +/* { dg-require-effective-target arm_thumb1_ok } */ + +struct S +{ + short x, y; +}; + +void foo (struct S *p, struct S *q, char *t, int n) +{ + struct S *c, d; + int x = 1; + + while (n--) + { + if (*t && p) + c = p; + q->x = d.x + c->x + c->y; + if (x) + { + x = 0; + d.x += c->x; + } + } +}