Re: [PATCH] Fix combine ICE on PPC

[Franz Sirl <Franz dot Sirl-kernel at lauterbach dot com>]

At 01:32 22.01.2002, Alan Modra wrote:
>On Mon, Jan 21, 2002 at 08:08:58PM +0100, Franz Sirl wrote:
>
> > Index: gcc/combine.c
> > ===================================================================
> > RCS file: /cvs/gcc/gcc/gcc/combine.c,v
> > retrieving revision 1.258
> > diff -u -p -r1.258 combine.c
> > --- gcc/combine.c     2002/01/18 13:40:27     1.258
> > +++ gcc/combine.c     2002/01/21 19:06:14
> > @@ -7830,6 +7830,7 @@ simplify_and_const_int (x, mode, varop,
> >
> >    else
> >      {
> > +      constop = trunc_int_for_mode (constop, mode);
> >        if (GET_CODE (XEXP (x, 1)) != CONST_INT
> >         || (unsigned HOST_WIDE_INT) INTVAL (XEXP (x, 1)) != constop)
> >       SUBST (XEXP (x, 1), GEN_INT (constop));
>
>I've had a similar patch in my powerpc64-linux gcc patchset for a while,
>and hadn't got around to submitting it yet.  Shouldn't this go up a
>few lines?  ie. before the GEN_INT (constop) in gen_binary (AND, ..
>
>Also, I reckon the trunc_int_for_mode earlier on "nonzero" is a mistake.
>When the sign bit for "mode" happens to be set, "nonzero" will be
>left-filled with one bits, which could cause some optimizations to
>be missed.  There's another missed optimization in force_to_mode,
>where sign-extended constants cause a mis-match.  Found when back-
>porting gcc-3.1 sign-extension of const_ints to gcc-3.0, and noticing
>that the powerpc64 linux kernel code got worse in some places.
>
>         * combine.c (simplify_and_const_int): Don't trunc_int_for_mode
>         "nonzero" as that might add "1" bits.  Ensure "constop" is
>         properly sign extened.
>         (force_to_mode): Tweak for sign extended constop.

Hmm, I did go for the minimum patch solving the ICE :-), but your patch 
looks reasonable. I can't approve it though :-).

BTW, while browsing thru combine.c I stumbled over this in merge_outer_ops:

---
       case XOR:
         if (op1 == AND)
           /* (a & b) ^ b == (~a) & b */
           op0 = AND, *pcomp_p = 1;
         else /* op1 == IOR */
           /* (a | b) ^ b == a & ~b */
           op0 = AND, *pconst0 = ~const0;
         break;

       case AND:
         if (op1 == IOR)
           /* (a | b) & b == b */
         op0 = SET;
         else /* op1 == XOR */
           /* (a ^ b) & b) == (~a) & b */
           *pcomp_p = 1;
         break;
       default:
         break;
       }

   /* Check for NO-OP cases.  */
   const0 &= GET_MODE_MASK (mode);
   if (const0 == 0
       && (op0 == IOR || op0 == XOR || op0 == PLUS))
     op0 = NIL;
   else if (const0 == 0 && op0 == AND)
     op0 = SET;
   else if ((unsigned HOST_WIDE_INT) const0 == GET_MODE_MASK (mode)
            && op0 == AND)
     op0 = NIL;

   /* ??? Slightly redundant with the above mask, but not entirely.
      Moving this above means we'd have to sign-extend the mode mask
      for the final test.  */
   const0 = trunc_int_for_mode (const0, mode);

   *pop0 = op0;
   *pconst0 = const0;
---

Isn't the *pconst0 = ~const0 a complete NO-OP here?

Franz.