RE: [RFC] Further LRA subreg handling issues

[Matthew Fortune <Matthew dot Fortune at imgtec dot com>]

Vladimir Makarov <vmakarov@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