Vladimir Makarov <vmaka...@redhat.com> writes: > On 01/16/2017 10:47 AM, Matthew Fortune wrote: > > Hi Vladimir, > > > > I'm working on PR target/78660 which is looking like a latent LRA bug. > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660 > > > > I believe the problem is in the same area as a bug was fixed in 2015: > > > > https://gcc.gnu.org/ml/gcc-patches/2015-01/msg02165.html > > > > Eric pointed out that the new issue relates to something reload > > specifically dealt with in reload1.c:eliminate_regs_1: > > > > if (MEM_P (new_rtx) > > && ((x_size < new_size > > /* On RISC machines, combine can create rtl of the form > > (set (subreg:m1 (reg:m2 R) 0) ...) > > where m1 < m2, and expects something interesting to > > happen to the entire word. Moreover, it will use the > > (reg:m2 R) later, expecting all bits to be preserved. > > So if the number of words is the same, preserve the > > subreg so that push_reload can see it. */ > > && !(WORD_REGISTER_OPERATIONS > > && (x_size - 1) / UNITS_PER_WORD > > == (new_size -1 ) / UNITS_PER_WORD)) > > || x_size == new_size) > > ) > > return adjust_address_nv (new_rtx, GET_MODE (x), SUBREG_BYTE (x)); > > else > > return gen_rtx_SUBREG (GET_MODE (x), new_rtx, SUBREG_BYTE (x)); > > > > However the code in lra-constraints.c:curr_insn_transform does not appear > > to make any attempt to handle a special case for WORD_REGISTER_OPERATIONS. > > I tried the following patch to account for this, which 'works' but I'm not > > at all sure what the conditions should be (the comment from reload will > > need adapting and including as well): > > > > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c > > index 260591a..ac8d116 100644 > > --- a/gcc/lra-constraints.c > > +++ b/gcc/lra-constraints.c > > @@ -4086,7 +4086,9 @@ curr_insn_transform (bool check_only_p) > > && (goal_alt[i] == NO_REGS > > || (simplify_subreg_regno > > (ira_class_hard_regs[goal_alt[i]][0], > > - GET_MODE (reg), byte, mode) >= 0))))) > > + GET_MODE (reg), byte, mode) >= 0))) > > + || (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (reg)) > > + && WORD_REGISTER_OPERATIONS))) > > { > > if (type == OP_OUT) > > type = OP_INOUT; > > > > I think at the very least the issue Richard pointed out in the previous > > fix must be dealt with as the new testcase triggers exactly what he > > described I believe > > > > Richard Sandiford wrote: > >> So IMO the patch is too broad. I think it should only use INOUT reloads > >> for !strict_low if the inner mode is wider than a word and the outer mode > >> is strictly narrower than the inner mode. That's on top of Vlad's > >> comment about only converting OP_OUTs, of course. > > And here is my attempt at dealing with that: > > > > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c > > index ac8d116..8a0f40f 100644 > > --- a/gcc/lra-constraints.c > > +++ b/gcc/lra-constraints.c > > @@ -4090,7 +4090,17 @@ curr_insn_transform (bool check_only_p) > > || (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (reg)) > > && WORD_REGISTER_OPERATIONS))) > > { > > - if (type == OP_OUT) > > + /* An OP_INOUT is required when reloading a subreg of a > > + mode wider than a word to ensure that data beyond the > > + word being reloaded is preserved. Also automatically > > + ensure that strict_low_part reloads are made into > > + OP_INOUT which should already be true from the backend > > + constraints. */ > > + if (type == OP_OUT > > + && (curr_static_id->operand[i].strict_low > > + || (GET_MODE_SIZE (GET_MODE (reg)) > UNITS_PER_WORD > > + && GET_MODE_SIZE (mode) > > + < GET_MODE_SIZE (GET_MODE (reg))))) > > type = OP_INOUT; > > loc = &SUBREG_REG (*loc); > > mode = GET_MODE (*loc); > > > > Any thoughts on whether this is along the right track would be appreciated. > > > > > Mathew, sorry for the delay with the answer. I needed some time to > think about it. Dealing with subregs in lra/reload is a complicated and > sensitive area. > > The patch looks ok for me. You can commit it if of course there are no > regressions. I hope the patch will behave well but please, be prepared > to work more on it if there are complications. Sometimes such changes on > LRA need a few iterations.
Thanks for taking a look. I will do wider testing before commit and I'm certainly braced for further complications. I unfortunately need to try and address another LRA issue hitting MIPS as well so I have a lot of testing to do. I'll run testing for at least x86_64, MIPS and another WORD_REGISTER_OPERATIONS target and try to get this committed in the next couple of days so it can get into everyone's testing well before release. Thanks, Matthew
