Re: [PATCH] combine: Improve change_zero_ext (fixes PR71847)

[Jeff Law <law at redhat dot com>]

On 11/23/2016 09:53 AM, Georg-Johann Lay wrote:
> Segher Boessenkool schrieb:
> > On Wed, Nov 23, 2016 at 04:58:22PM +0100, Georg-Johann Lay wrote:
> > > Hi, this causes an illegal code issue on avr.
> >
> > Sorry about that.
> >
> > [ snip ]
> >
> > > Trying 19 -> 7:
> > > Failed to match this instruction:
> > > (set (reg:HI 45 [ x+3 ])
> > >     (zero_extend:HI (reg:QI 25 r25 [ x+3 ])))
> > > Successfully matched this instruction:
> > > (set (reg:HI 45 [ x+3 ])
> > >     (and:HI (reg:HI 25 r25)
> > >         (const_int 255 [0xff])))
> > > allowing combination of insns 19 and 7
> >
> > > R25 is a 16-bit hard reg, but HImode (and larger) registers must
> > > start at an even register number.
> >
> > So why does the define_insn allow it?
>
> Because the insn predicate is register_operand:HI which should be fine
> as it is non-strict RTL.  Or are predicates supposed to reject such odd
> operands the backend would never generate itself?
>
> Are you saying that register_operand:HI is not correct?
>
> IMO, if combine is playing with hard regs, it should be conservative and
> not create new hard regs out of thin air...  like
>
> * Test HARD_REGNO_MODE_OK
>
> * Don't increase HARD_REGNO_NREGS because it's not a good idea to create
> hard regs out of thin air.  On avr, for example, one should strive to
> get to smaller modes, not to blow mode sizes...
There's already some code in the combiner to avoid some of these 
situations.  For example:

     /* If register alignment is being enforced for multi-word items in all
         cases except for parameters, it is possible to have a register 
copy
         insn referencing a hard register that is not allowed to 
contain the
         mode being copied and which would not be valid as an operand 
of most
         insns.  Eliminate this problem by not combining with such an insn.

         Also, on some machines we don't want to extend the life of a hard
         register.  */

      if (REG_P (src)
          && ((REGNO (dest) < FIRST_PSEUDO_REGISTER
               && ! HARD_REGNO_MODE_OK (REGNO (dest), GET_MODE (dest)))
              /* Don't extend the life of a hard register unless it is
                 user variable (if we have few registers) or it can't
                 fit into the desired register (meaning something special
                 is going on).
                 Also avoid substituting a return register into I3, because
                 reload can't handle a conflict with constraints of other
                 inputs.  */
              || (REGNO (src) < FIRST_PSEUDO_REGISTER
                  && ! HARD_REGNO_MODE_OK (REGNO (src), GET_MODE (src)))))
        return 0;



Jeff