| Message ID | 4F994B99.1010606@arm.com |
|---|---|
| State | New |
| Headers | show |
On 26/04/12 14:20, Jim MacArthur wrote: > The current code in reg_fits_class_p appears to be incorrect; since > offset may be negative, it's necessary to check both ends of the range > otherwise an array overrun or underrun may occur when calling > in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each > register in the range of regno .. regno+offset. > > A negative offset can occur on a big-endian target. For example, on > AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. > > We discovered this problem while developing unrelated code for > big-endian support in the AArch64 back end. > > I've tested this with an x86 bootstrap which shows no errors, and with > our own AArch64 back end. > > Ok for trunk? > > gcc/Changelog entry: > > 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> > * recog.c (reg_fits_class_p): Check every register between regno and > regno+offset is in the hard register set. > OK. R. > > reg-fits-class-9 > > > diff --git a/gcc/recog.c b/gcc/recog.c > index 8fb96a0..825bfb1 100644 > --- a/gcc/recog.c > +++ b/gcc/recog.c > @@ -2759,14 +2759,28 @@ bool > reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, > enum machine_mode mode) > { > - int regno = REGNO (operand); > + unsigned int regno = REGNO (operand); > > if (cl == NO_REGS) > return false; > > - return (HARD_REGISTER_NUM_P (regno) > - && in_hard_reg_set_p (reg_class_contents[(int) cl], > - mode, regno + offset)); > + /* We should not assume all registers in the range regno to regno + offset are > + valid just because each end of the range is. */ > + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) > + { > + unsigned int i; > + > + unsigned int start = MIN (regno, regno + offset); > + unsigned int end = MAX (regno, regno + offset); > + for (i = start; i <= end; i++) > + { > + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], > + mode, i)) > + return false; > + } > + return true; > + } > + return false; > } > > /* Split single instruction. Helper function for split_all_insns and
Richard Earnshaw <rearnsha@arm.com> writes: > On 26/04/12 14:20, Jim MacArthur wrote: >> The current code in reg_fits_class_p appears to be incorrect; since >> offset may be negative, it's necessary to check both ends of the range >> otherwise an array overrun or underrun may occur when calling >> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >> register in the range of regno .. regno+offset. >> >> A negative offset can occur on a big-endian target. For example, on >> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >> >> We discovered this problem while developing unrelated code for >> big-endian support in the AArch64 back end. >> >> I've tested this with an x86 bootstrap which shows no errors, and with >> our own AArch64 back end. >> >> Ok for trunk? >> >> gcc/Changelog entry: >> >> 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >> * recog.c (reg_fits_class_p): Check every register between regno and >> regno+offset is in the hard register set. >> > > OK. > > R. > >> >> reg-fits-class-9 >> >> >> diff --git a/gcc/recog.c b/gcc/recog.c >> index 8fb96a0..825bfb1 100644 >> --- a/gcc/recog.c >> +++ b/gcc/recog.c >> @@ -2759,14 +2759,28 @@ bool >> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >> enum machine_mode mode) >> { >> - int regno = REGNO (operand); >> + unsigned int regno = REGNO (operand); >> >> if (cl == NO_REGS) >> return false; >> >> - return (HARD_REGISTER_NUM_P (regno) >> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >> - mode, regno + offset)); >> + /* We should not assume all registers in the range regno to regno + offset are >> + valid just because each end of the range is. */ >> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >> + { >> + unsigned int i; >> + >> + unsigned int start = MIN (regno, regno + offset); >> + unsigned int end = MAX (regno, regno + offset); >> + for (i = start; i <= end; i++) >> + { >> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >> + mode, i)) >> + return false; >> + } This doesn't look right to me. We should still only need to check in_hard_reg_set_p for one register number. I'd have expected something like: return (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset) && in_hard_reg_set_p (reg_class_contents[(int) cl], mode, regno + offset)); instead. Richard
On 30/04/12 15:07, Richard Sandiford wrote: > Richard Earnshaw <rearnsha@arm.com> writes: >> On 26/04/12 14:20, Jim MacArthur wrote: >>> The current code in reg_fits_class_p appears to be incorrect; since >>> offset may be negative, it's necessary to check both ends of the range >>> otherwise an array overrun or underrun may occur when calling >>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >>> register in the range of regno .. regno+offset. >>> >>> A negative offset can occur on a big-endian target. For example, on >>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>> >>> We discovered this problem while developing unrelated code for >>> big-endian support in the AArch64 back end. >>> >>> I've tested this with an x86 bootstrap which shows no errors, and with >>> our own AArch64 back end. >>> >>> Ok for trunk? >>> >>> gcc/Changelog entry: >>> >>> 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >>> * recog.c (reg_fits_class_p): Check every register between regno and >>> regno+offset is in the hard register set. >>> >> >> OK. >> >> R. >> >>> >>> reg-fits-class-9 >>> >>> >>> diff --git a/gcc/recog.c b/gcc/recog.c >>> index 8fb96a0..825bfb1 100644 >>> --- a/gcc/recog.c >>> +++ b/gcc/recog.c >>> @@ -2759,14 +2759,28 @@ bool >>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>> enum machine_mode mode) >>> { >>> - int regno = REGNO (operand); >>> + unsigned int regno = REGNO (operand); >>> >>> if (cl == NO_REGS) >>> return false; >>> >>> - return (HARD_REGISTER_NUM_P (regno) >>> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >>> - mode, regno + offset)); >>> + /* We should not assume all registers in the range regno to regno + offset are >>> + valid just because each end of the range is. */ >>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >>> + { >>> + unsigned int i; >>> + >>> + unsigned int start = MIN (regno, regno + offset); >>> + unsigned int end = MAX (regno, regno + offset); >>> + for (i = start; i <= end; i++) >>> + { >>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>> + mode, i)) >>> + return false; >>> + } > > This doesn't look right to me. We should still only need to check > in_hard_reg_set_p for one register number. I'd have expected > something like: > > return (HARD_REGISTER_NUM_P (regno) > && HARD_REGISTER_NUM_P (regno + offset) > && in_hard_reg_set_p (reg_class_contents[(int) cl], > mode, regno + offset)); > > instead. > > Richard > There's no guarantee that all registers in a set are contiguous; ARM for example doesn't make that guarantee, since SP is not a GP register, but R12 and R14 are. R.
On 30/04/12 15:07, Richard Sandiford wrote: > Richard Earnshaw <rearnsha@arm.com> writes: >> On 26/04/12 14:20, Jim MacArthur wrote: >>> The current code in reg_fits_class_p appears to be incorrect; since >>> offset may be negative, it's necessary to check both ends of the range >>> otherwise an array overrun or underrun may occur when calling >>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >>> register in the range of regno .. regno+offset. >>> >>> A negative offset can occur on a big-endian target. For example, on >>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>> >>> We discovered this problem while developing unrelated code for >>> big-endian support in the AArch64 back end. >>> >>> I've tested this with an x86 bootstrap which shows no errors, and with >>> our own AArch64 back end. >>> >>> Ok for trunk? >>> >>> gcc/Changelog entry: >>> >>> 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >>> * recog.c (reg_fits_class_p): Check every register between regno and >>> regno+offset is in the hard register set. >>> >> >> OK. >> >> R. >> >>> >>> reg-fits-class-9 >>> >>> >>> diff --git a/gcc/recog.c b/gcc/recog.c >>> index 8fb96a0..825bfb1 100644 >>> --- a/gcc/recog.c >>> +++ b/gcc/recog.c >>> @@ -2759,14 +2759,28 @@ bool >>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>> enum machine_mode mode) >>> { >>> - int regno = REGNO (operand); >>> + unsigned int regno = REGNO (operand); >>> >>> if (cl == NO_REGS) >>> return false; >>> >>> - return (HARD_REGISTER_NUM_P (regno) >>> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >>> - mode, regno + offset)); >>> + /* We should not assume all registers in the range regno to regno + offset are >>> + valid just because each end of the range is. */ >>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >>> + { >>> + unsigned int i; >>> + >>> + unsigned int start = MIN (regno, regno + offset); >>> + unsigned int end = MAX (regno, regno + offset); >>> + for (i = start; i <= end; i++) >>> + { >>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>> + mode, i)) >>> + return false; >>> + } > > This doesn't look right to me. We should still only need to check > in_hard_reg_set_p for one register number. I'd have expected > something like: > > return (HARD_REGISTER_NUM_P (regno) > && HARD_REGISTER_NUM_P (regno + offset) > && in_hard_reg_set_p (reg_class_contents[(int) cl], > mode, regno + offset)); > > instead. > > Richard > Hmm, looking at the comment that precedes the function makes me think the actual implementation should be: { int regno = REGNO (operand) + offset; ... return (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (end_hard_regno (regno, mode)) && in_hard_reg_set_p (...., regno); } That is, the original regno isn't really interesting and what really counts is the range regno + offset ... regno + offset + NUM_HARD_REGS(mode) - 1 R.
Richard Earnshaw <rearnsha@arm.com> writes: > On 30/04/12 15:07, Richard Sandiford wrote: >> Richard Earnshaw <rearnsha@arm.com> writes: >>> On 26/04/12 14:20, Jim MacArthur wrote: >>>> The current code in reg_fits_class_p appears to be incorrect; since >>>> offset may be negative, it's necessary to check both ends of the range >>>> otherwise an array overrun or underrun may occur when calling >>>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >>>> register in the range of regno .. regno+offset. >>>> >>>> A negative offset can occur on a big-endian target. For example, on >>>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>>> >>>> We discovered this problem while developing unrelated code for >>>> big-endian support in the AArch64 back end. >>>> >>>> I've tested this with an x86 bootstrap which shows no errors, and with >>>> our own AArch64 back end. >>>> >>>> Ok for trunk? >>>> >>>> gcc/Changelog entry: >>>> >>>> 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >>>> * recog.c (reg_fits_class_p): Check every register between regno and >>>> regno+offset is in the hard register set. >>>> >>> >>> OK. >>> >>> R. >>> >>>> >>>> reg-fits-class-9 >>>> >>>> >>>> diff --git a/gcc/recog.c b/gcc/recog.c >>>> index 8fb96a0..825bfb1 100644 >>>> --- a/gcc/recog.c >>>> +++ b/gcc/recog.c >>>> @@ -2759,14 +2759,28 @@ bool >>>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>>> enum machine_mode mode) >>>> { >>>> - int regno = REGNO (operand); >>>> + unsigned int regno = REGNO (operand); >>>> >>>> if (cl == NO_REGS) >>>> return false; >>>> >>>> - return (HARD_REGISTER_NUM_P (regno) >>>> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >>>> - mode, regno + offset)); >>>> + /* We should not assume all registers in the range regno to regno + offset are >>>> + valid just because each end of the range is. */ >>>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >>>> + { >>>> + unsigned int i; >>>> + >>>> + unsigned int start = MIN (regno, regno + offset); >>>> + unsigned int end = MAX (regno, regno + offset); >>>> + for (i = start; i <= end; i++) >>>> + { >>>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>>> + mode, i)) >>>> + return false; >>>> + } >> >> This doesn't look right to me. We should still only need to check >> in_hard_reg_set_p for one register number. I'd have expected >> something like: >> >> return (HARD_REGISTER_NUM_P (regno) >> && HARD_REGISTER_NUM_P (regno + offset) >> && in_hard_reg_set_p (reg_class_contents[(int) cl], >> mode, regno + offset)); >> >> instead. >> >> Richard >> > > There's no guarantee that all registers in a set are contiguous; ARM for > example doesn't make that guarantee, since SP is not a GP register, but > R12 and R14 are. Sorry, I don't follow. My point was that in_hard_reg_set_p (C, M, R1) tests whether every register required to store a value of mode M starting at R1 fits in C. Which is what we want to know. Whether the intermediate registers (between regno and regno + offset) are even valid for MODE shouldn't matter. I don't think it makes conceptual sense to call: if (!in_hard_reg_set_p (reg_class_contents[(int) cl], mode, i)) for regno < i < regno + offset (or regno + offset < i < regno), because we're not trying to construct a value of mode MODE in that register. Richard
On 30/04/12 15:39, Richard Sandiford wrote: > Richard Earnshaw <rearnsha@arm.com> writes: >> On 30/04/12 15:07, Richard Sandiford wrote: >>> Richard Earnshaw <rearnsha@arm.com> writes: >>>> On 26/04/12 14:20, Jim MacArthur wrote: >>>>> The current code in reg_fits_class_p appears to be incorrect; since >>>>> offset may be negative, it's necessary to check both ends of the range >>>>> otherwise an array overrun or underrun may occur when calling >>>>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >>>>> register in the range of regno .. regno+offset. >>>>> >>>>> A negative offset can occur on a big-endian target. For example, on >>>>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>>>> >>>>> We discovered this problem while developing unrelated code for >>>>> big-endian support in the AArch64 back end. >>>>> >>>>> I've tested this with an x86 bootstrap which shows no errors, and with >>>>> our own AArch64 back end. >>>>> >>>>> Ok for trunk? >>>>> >>>>> gcc/Changelog entry: >>>>> >>>>> 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >>>>> * recog.c (reg_fits_class_p): Check every register between regno and >>>>> regno+offset is in the hard register set. >>>>> >>>> >>>> OK. >>>> >>>> R. >>>> >>>>> >>>>> reg-fits-class-9 >>>>> >>>>> >>>>> diff --git a/gcc/recog.c b/gcc/recog.c >>>>> index 8fb96a0..825bfb1 100644 >>>>> --- a/gcc/recog.c >>>>> +++ b/gcc/recog.c >>>>> @@ -2759,14 +2759,28 @@ bool >>>>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>>>> enum machine_mode mode) >>>>> { >>>>> - int regno = REGNO (operand); >>>>> + unsigned int regno = REGNO (operand); >>>>> >>>>> if (cl == NO_REGS) >>>>> return false; >>>>> >>>>> - return (HARD_REGISTER_NUM_P (regno) >>>>> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>> - mode, regno + offset)); >>>>> + /* We should not assume all registers in the range regno to regno + offset are >>>>> + valid just because each end of the range is. */ >>>>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >>>>> + { >>>>> + unsigned int i; >>>>> + >>>>> + unsigned int start = MIN (regno, regno + offset); >>>>> + unsigned int end = MAX (regno, regno + offset); >>>>> + for (i = start; i <= end; i++) >>>>> + { >>>>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>> + mode, i)) >>>>> + return false; >>>>> + } >>> >>> This doesn't look right to me. We should still only need to check >>> in_hard_reg_set_p for one register number. I'd have expected >>> something like: >>> >>> return (HARD_REGISTER_NUM_P (regno) >>> && HARD_REGISTER_NUM_P (regno + offset) >>> && in_hard_reg_set_p (reg_class_contents[(int) cl], >>> mode, regno + offset)); >>> >>> instead. >>> >>> Richard >>> >> >> There's no guarantee that all registers in a set are contiguous; ARM for >> example doesn't make that guarantee, since SP is not a GP register, but >> R12 and R14 are. > > Sorry, I don't follow. My point was that in_hard_reg_set_p (C, M, R1) > tests whether every register required to store a value of mode M starting > at R1 fits in C. Which is what we want to know. > > Whether the intermediate registers (between regno and regno + offset) > are even valid for MODE shouldn't matter. I don't think it makes > conceptual sense to call: > > if (!in_hard_reg_set_p (reg_class_contents[(int) cl], > mode, i)) > > for regno < i < regno + offset (or regno + offset < i < regno), > because we're not trying to construct a value of mode MODE > in that register. > > Richard > You're right, of course. I'd missed that in my initial review; and hence my follow-up suggestion. It's not particularly interesting whether or not HARD_REGISTER_NUM_P (REGNO (operand)) is true, only whether REGNO(operand) + offset ... REGNO(operand) + offset + NUM_REGS(mode) -1 is. R.
Richard Earnshaw <rearnsha@arm.com> writes: > On 30/04/12 15:39, Richard Sandiford wrote: >> Richard Earnshaw <rearnsha@arm.com> writes: >>> On 30/04/12 15:07, Richard Sandiford wrote: >>>> Richard Earnshaw <rearnsha@arm.com> writes: >>>>> On 26/04/12 14:20, Jim MacArthur wrote: >>>>>> The current code in reg_fits_class_p appears to be incorrect; since >>>>>> offset may be negative, it's necessary to check both ends of the range >>>>>> otherwise an array overrun or underrun may occur when calling >>>>>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >>>>>> register in the range of regno .. regno+offset. >>>>>> >>>>>> A negative offset can occur on a big-endian target. For example, on >>>>>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>>>>> >>>>>> We discovered this problem while developing unrelated code for >>>>>> big-endian support in the AArch64 back end. >>>>>> >>>>>> I've tested this with an x86 bootstrap which shows no errors, and with >>>>>> our own AArch64 back end. >>>>>> >>>>>> Ok for trunk? >>>>>> >>>>>> gcc/Changelog entry: >>>>>> >>>>>> 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >>>>>> * recog.c (reg_fits_class_p): Check every register between regno and >>>>>> regno+offset is in the hard register set. >>>>>> >>>>> >>>>> OK. >>>>> >>>>> R. >>>>> >>>>>> >>>>>> reg-fits-class-9 >>>>>> >>>>>> >>>>>> diff --git a/gcc/recog.c b/gcc/recog.c >>>>>> index 8fb96a0..825bfb1 100644 >>>>>> --- a/gcc/recog.c >>>>>> +++ b/gcc/recog.c >>>>>> @@ -2759,14 +2759,28 @@ bool >>>>>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>>>>> enum machine_mode mode) >>>>>> { >>>>>> - int regno = REGNO (operand); >>>>>> + unsigned int regno = REGNO (operand); >>>>>> >>>>>> if (cl == NO_REGS) >>>>>> return false; >>>>>> >>>>>> - return (HARD_REGISTER_NUM_P (regno) >>>>>> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>>> - mode, regno + offset)); >>>>>> + /* We should not assume all registers in the range regno to regno + offset are >>>>>> + valid just because each end of the range is. */ >>>>>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >>>>>> + { >>>>>> + unsigned int i; >>>>>> + >>>>>> + unsigned int start = MIN (regno, regno + offset); >>>>>> + unsigned int end = MAX (regno, regno + offset); >>>>>> + for (i = start; i <= end; i++) >>>>>> + { >>>>>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>>> + mode, i)) >>>>>> + return false; >>>>>> + } >>>> >>>> This doesn't look right to me. We should still only need to check >>>> in_hard_reg_set_p for one register number. I'd have expected >>>> something like: >>>> >>>> return (HARD_REGISTER_NUM_P (regno) >>>> && HARD_REGISTER_NUM_P (regno + offset) >>>> && in_hard_reg_set_p (reg_class_contents[(int) cl], >>>> mode, regno + offset)); >>>> >>>> instead. >>>> >>>> Richard >>>> >>> >>> There's no guarantee that all registers in a set are contiguous; ARM for >>> example doesn't make that guarantee, since SP is not a GP register, but >>> R12 and R14 are. >> >> Sorry, I don't follow. My point was that in_hard_reg_set_p (C, M, R1) >> tests whether every register required to store a value of mode M starting >> at R1 fits in C. Which is what we want to know. >> >> Whether the intermediate registers (between regno and regno + offset) >> are even valid for MODE shouldn't matter. I don't think it makes >> conceptual sense to call: >> >> if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >> mode, i)) >> >> for regno < i < regno + offset (or regno + offset < i < regno), >> because we're not trying to construct a value of mode MODE >> in that register. >> >> Richard >> > > > You're right, of course. I'd missed that in my initial review; and > hence my follow-up suggestion. It's not particularly interesting > whether or not HARD_REGISTER_NUM_P (REGNO (operand)) is true, only > whether REGNO(operand) + offset ... REGNO(operand) + offset + > NUM_REGS(mode) -1 is. The problem is that the function currently accepts pseudo registers. We wouldn't want FIRST_PSEUDO_REGISTER to be accidentally associated with FIRST_PSUEDO_REGISTER-1, however unlikely that combination of arguments might be in practice. I think the HARD_REGISTER_NUM_P check is needed for both regno and regno + offset. I agree that we should protect against overrun in in_hard_reg_set, but I think that check logically belongs there. Most targets happen to be OK because FIRST_PSEUDO_REGISTER isn't divisible by 32, so the class HARD_REG_SETs always have a terminating zero bit. There are a couple of exceptions though, such as alpha and lm32. So one fix would be to require HARD_REG_SET to have an entry for FIRST_PSEUDO_REGISTER, which wouldn't affect most targets. Another would be to have an explicit range check in in_hard_reg_set_p and friends. Richard
Richard Earnshaw schrieb: > On 30/04/12 15:07, Richard Sandiford wrote: > >>Richard Earnshaw writes: >> >>> Jim MacArthur wrote: >>> >>>>The current code in reg_fits_class_p appears to be incorrect; since >>>>offset may be negative, it's necessary to check both ends of the range >>>>otherwise an array overrun or underrun may occur when calling >>>>in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >>>>register in the range of regno .. regno+offset. >>>> >>>>A negative offset can occur on a big-endian target. For example, on >>>>AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>>> >>>>We discovered this problem while developing unrelated code for >>>>big-endian support in the AArch64 back end. >>>> >>>>I've tested this with an x86 bootstrap which shows no errors, and with >>>>our own AArch64 back end. >>>> >>>>Ok for trunk? >>>> >>>>gcc/Changelog entry: >>>> >>>>2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >>>> * recog.c (reg_fits_class_p): Check every register between regno and >>>> regno+offset is in the hard register set. >>> >>>OK. >>> >>>R. >>> >>>>reg-fits-class-9 >>>> >>>>diff --git a/gcc/recog.c b/gcc/recog.c >>>>index 8fb96a0..825bfb1 100644 >>>>--- a/gcc/recog.c >>>>+++ b/gcc/recog.c >>>>@@ -2759,14 +2759,28 @@ bool >>>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>>> enum machine_mode mode) >>>> { >>>>- int regno = REGNO (operand); >>>>+ unsigned int regno = REGNO (operand); >>>> >>>> if (cl == NO_REGS) >>>> return false; >>>> >>>>- return (HARD_REGISTER_NUM_P (regno) >>>>- && in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>- mode, regno + offset)); >>>>+ /* We should not assume all registers in the range regno to regno + offset are >>>>+ valid just because each end of the range is. */ >>>>+ if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >>>>+ { >>>>+ unsigned int i; >>>>+ >>>>+ unsigned int start = MIN (regno, regno + offset); >>>>+ unsigned int end = MAX (regno, regno + offset); >>>>+ for (i = start; i <= end; i++) >>>>+ { >>>>+ if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>+ mode, i)) >>>>+ return false; >>>>+ } >> >>This doesn't look right to me. We should still only need to check >>in_hard_reg_set_p for one register number. I'd have expected >>something like: >> >> return (HARD_REGISTER_NUM_P (regno) >> && HARD_REGISTER_NUM_P (regno + offset) >> && in_hard_reg_set_p (reg_class_contents[(int) cl], >> mode, regno + offset)); >> >>instead. >> >>Richard > > Hmm, looking at the comment that precedes the function makes me think > the actual implementation should be: > > { > int regno = REGNO (operand) + offset; > ... > > return (HARD_REGISTER_NUM_P (regno) > && HARD_REGISTER_NUM_P (end_hard_regno (regno, mode)) > && in_hard_reg_set_p (...., regno); > > } > > That is, the original regno isn't really interesting and what really > counts is the range regno + offset ... regno + offset + > NUM_HARD_REGS(mode) - 1 Shouldn't this be HARD_REGNO_NREGS? Johann
On 30/04/12 16:36, Georg-Johann Lay wrote: > Richard Earnshaw schrieb: >> On 30/04/12 15:07, Richard Sandiford wrote: >> >>> Richard Earnshaw writes: >>> >>>> Jim MacArthur wrote: >>>> >>>>> The current code in reg_fits_class_p appears to be incorrect; since >>>>> offset may be negative, it's necessary to check both ends of the range >>>>> otherwise an array overrun or underrun may occur when calling >>>>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each >>>>> register in the range of regno .. regno+offset. >>>>> >>>>> A negative offset can occur on a big-endian target. For example, on >>>>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>>>> >>>>> We discovered this problem while developing unrelated code for >>>>> big-endian support in the AArch64 back end. >>>>> >>>>> I've tested this with an x86 bootstrap which shows no errors, and with >>>>> our own AArch64 back end. >>>>> >>>>> Ok for trunk? >>>>> >>>>> gcc/Changelog entry: >>>>> >>>>> 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> >>>>> * recog.c (reg_fits_class_p): Check every register between regno and >>>>> regno+offset is in the hard register set. >>>> >>>> OK. >>>> >>>> R. >>>> >>>>> reg-fits-class-9 >>>>> >>>>> diff --git a/gcc/recog.c b/gcc/recog.c >>>>> index 8fb96a0..825bfb1 100644 >>>>> --- a/gcc/recog.c >>>>> +++ b/gcc/recog.c >>>>> @@ -2759,14 +2759,28 @@ bool >>>>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>>>> enum machine_mode mode) >>>>> { >>>>> - int regno = REGNO (operand); >>>>> + unsigned int regno = REGNO (operand); >>>>> >>>>> if (cl == NO_REGS) >>>>> return false; >>>>> >>>>> - return (HARD_REGISTER_NUM_P (regno) >>>>> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>> - mode, regno + offset)); >>>>> + /* We should not assume all registers in the range regno to regno + offset are >>>>> + valid just because each end of the range is. */ >>>>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) >>>>> + { >>>>> + unsigned int i; >>>>> + >>>>> + unsigned int start = MIN (regno, regno + offset); >>>>> + unsigned int end = MAX (regno, regno + offset); >>>>> + for (i = start; i <= end; i++) >>>>> + { >>>>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>> + mode, i)) >>>>> + return false; >>>>> + } >>> >>> This doesn't look right to me. We should still only need to check >>> in_hard_reg_set_p for one register number. I'd have expected >>> something like: >>> >>> return (HARD_REGISTER_NUM_P (regno) >>> && HARD_REGISTER_NUM_P (regno + offset) >>> && in_hard_reg_set_p (reg_class_contents[(int) cl], >>> mode, regno + offset)); >>> >>> instead. >>> >>> Richard >> >> Hmm, looking at the comment that precedes the function makes me think >> the actual implementation should be: >> >> { >> int regno = REGNO (operand) + offset; >> ... >> >> return (HARD_REGISTER_NUM_P (regno) >> && HARD_REGISTER_NUM_P (end_hard_regno (regno, mode)) >> && in_hard_reg_set_p (...., regno); >> >> } >> >> That is, the original regno isn't really interesting and what really >> counts is the range regno + offset ... regno + offset + >> NUM_HARD_REGS(mode) - 1 > > Shouldn't this be HARD_REGNO_NREGS? > > Johann > Look at the definition of end_hard_regno... R.
diff --git a/gcc/recog.c b/gcc/recog.c index 8fb96a0..825bfb1 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -2759,14 +2759,28 @@ bool reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, enum machine_mode mode) { - int regno = REGNO (operand); + unsigned int regno = REGNO (operand); if (cl == NO_REGS) return false; - return (HARD_REGISTER_NUM_P (regno) - && in_hard_reg_set_p (reg_class_contents[(int) cl], - mode, regno + offset)); + /* We should not assume all registers in the range regno to regno + offset are + valid just because each end of the range is. */ + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset)) + { + unsigned int i; + + unsigned int start = MIN (regno, regno + offset); + unsigned int end = MAX (regno, regno + offset); + for (i = start; i <= end; i++) + { + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], + mode, i)) + return false; + } + return true; + } + return false; } /* Split single instruction. Helper function for split_all_insns and
The current code in reg_fits_class_p appears to be incorrect; since offset may be negative, it's necessary to check both ends of the range otherwise an array overrun or underrun may occur when calling in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each register in the range of regno .. regno+offset. A negative offset can occur on a big-endian target. For example, on AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. We discovered this problem while developing unrelated code for big-endian support in the AArch64 back end. I've tested this with an x86 bootstrap which shows no errors, and with our own AArch64 back end. Ok for trunk? gcc/Changelog entry: 2012-04-26 Jim MacArthur<jim.macarthur@arm.com> * recog.c (reg_fits_class_p): Check every register between regno and regno+offset is in the hard register set.