Re: RFC: LRA for x86/x86-64 [6/9]

[Vladimir Makarov <vmakarov at redhat dot com>]

On 12-09-28 4:07 PM, Jeff Law wrote:
> On 09/27/2012 04:58 PM, Vladimir Makarov wrote:
> >    The following patch modifies some code in the rest of compiler for
> > correct work of LRA.  The code works the same way when LRA is not
> > used.  It is achieved by checking a new variable lra_in_progress.
> >
> > 2012-09-27  Vladimir Makarov <vmakarov@redhat.com>
> >
> >      * rtlanal.c (simplify_subreg_regno): Permit ARG_POINTER_REGNUM and
> >      STACK_POINTER_REGNU for LRA.
> >      * jump.c (true_regnum): Always use hard_regno for 
> > subreg_get_info when
> >      lra is in progress.
> >      * expr.c (emit_move_insn_1): Pass an additional argument to
> >      emit_move_via_integer.  Use emit_move_via_integer for LRA only if
> >      the insn is recognized.
> >      * recog.c (general_operand, register_operand): Accept paradoxical
> > FLOAD_MODE
> >      subregs for LRA.
> >      (scratch_operand): Accept pseudos for LRA.
> >      * emit-rtl.c (gen_rtx_REG): Add lra_in_progress.
> >      (validate_subreg): Don't check offset for LRA and
> >      floating point modes.
> >      * rtl.h (lra_in_progress): New external.
> >      * ira.c (lra_in_progress): Define.
> s/FLOAD/FLOAT/ to fix ChangeLog typo.
Thanks.  I fixed it will be in the revised versions of the patches.
> > Index: jump.c
> > ===================================================================
> > --- jump.c    (revision 191771)
> > +++ jump.c    (working copy)
> > @@ -1868,7 +1868,8 @@ true_regnum (const_rtx x)
> >   {
> >     if (REG_P (x))
> >       {
> > -      if (REGNO (x) >= FIRST_PSEUDO_REGISTER && reg_renumber[REGNO 
> > (x)] >= 0)
> > +      if (REGNO (x) >= FIRST_PSEUDO_REGISTER
> > +      && (lra_in_progress || reg_renumber[REGNO (x)] >= 0))
> >       return reg_renumber[REGNO (x)];
> >         return REGNO (x);
> >       }
> This hunk doesn't make any sense to me, unless you want true_regnum to 
> return a negative value during LRA for cases where the pseudo is still 
> unassigned.  Is that what's you're intending here?  If that's what you 
> want, then I think it's worth a quick comment.
Yes, that was my intention.  LRA works a bit different from reload. It 
needs the current assignment of pseudos (assigned hard register or -1 
even it is not assigned).

I'll add the comment about this in the next version of the patch. The 
code looks a bit strange and could be more clear but I kept in my mind 
removing code for reload in the future.
> > @@ -1880,7 +1881,8 @@ true_regnum (const_rtx x)
> >       {
> >         struct subreg_info info;
> >
> > -      subreg_get_info (REGNO (SUBREG_REG (x)),
> > +      subreg_get_info (lra_in_progress
> > +               ? (unsigned) base : REGNO (SUBREG_REG (x)),
> >                  GET_MODE (SUBREG_REG (x)),
> >                  SUBREG_BYTE (x), GET_MODE (x), &info);
> I'd be good to indicate why you want to do something different for LRA 
> here.
I need to know the current allocation with taking into account that the 
subregister with final hard register will be representable. I'll add the 
comment.
> > Index: rtlanal.c
> > ===================================================================
> > --- rtlanal.c    (revision 191771)
> > +++ rtlanal.c    (working copy)
> > @@ -3465,7 +3465,9 @@ simplify_subreg_regno (unsigned int xreg
> >     /* Give the backend a chance to disallow the mode change. */
> >     if (GET_MODE_CLASS (xmode) != MODE_COMPLEX_INT
> >         && GET_MODE_CLASS (xmode) != MODE_COMPLEX_FLOAT
> > -      && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode))
> > +      && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode)
> > +      /* We can use mode change in LRA for some transformations.  */
> > +      && ! lra_in_progress)
> >       return -1;
> >   #endif
> I don't think this change is reflected in the ChangeLog.
You are right.  I added the change description in the ChangeLog. Reload 
has no problem in representation of some decisions because it uses 
internal representation.  It is a problem for LRA using RTL when for 
example two insn operands in different modes should have the same hard 
register according to the constraint.  I use subreg for that even it is 
not a correct in other parts of the compiler.
> I think just the minor ChangeLog updates and clarification of the 
> changes to jump.c are all that's needed for this patch to be approved.
Thanks, Jeff.  I really appreciate your help.