Re: target/5169: Bug in pa-risc gcc optimizer

[Klaus Pedersen <klaus dot k dot pedersen at nokia dot com>]

Thanks, your patch solves all of my problems! That means:

    target/5169: Bug in pa-risc gcc optimizer
    target/5185: Segmentation error in final.c

and as a bonus also this one:

    optimization/5264: Possible optimization bug at -O2 on HP-UX


With your patch applied to the snapshot gcc-20011231 I can bootstrap 
a working compiler gcc 2.95.3:

  ../gcc-20011231/configure --prefix=/... --enable-languages=c,c++ 
    --with-gnu-as --with-as=/opt/net/local/bin/as
  gmake bootstrap CFLAGS=-O1

Please note that if you are bootstrapping from gcc, then it is essencial
to make the first step of the bootstrap with optimizer level set to 
something lower than 2 (-O1 works).

With the native HP compiler this is probably not nessary.

Thanks, your patch might just solve years of mysterious problems! Now I
just 
have to build xemacs, to see if this solves the strange craches.

BR, Klaus



Alan Modra wrote:
> 
> On Fri, Dec 21, 2001 at 01:19:37PM -0000, Klaus.k.pedersen@nokia.com
> wrote:
> >
> > >Number:         5169
> > >Category:       target
> > >Synopsis:       Bug in pa-risc gcc optimizer
> > >Confidential:   no
> > >Severity:       serious
> > >Priority:       medium
> > >Responsible:    unassigned
> > >State:          open
> > >Class:          wrong-code
> > >Submitter-Id:   net
> > >Arrival-Date:   Fri Dec 21 05:26:01 PST 2001
> > >Closed-Date:
> > >Last-Modified:
> > >Originator:     Klaus Pedersen
> > >Release:        all - at least from 2.95 and up
> > >Organization:
> > >Environment:
> > HP-UX 11, HP-UX 10.20, gcc 2.95.2, to current snapshot
> >
> > >Description:
> > See also bootstrap/3479, closed by rodrigc@gcc.gnu.org
> > See also target/5107, investigated by Richard.Earnshaw@arm.com
> >
> > The attached program, which is the reason that it is
> > impossible to build a cross compiler for ARM on HP-UX
> > with default CFLAGS=-O2.
> >
> > Here is the program in all its glory:
> >
> > ----
> > int main()
> > {
> >     int operands[10] = {0, 65536};
> >     unsigned int val = operands[1];
> >     unsigned int mask = 0xff;
> >     int i;
> >
> >     for (i = 0; i < 25; i++)
> >       if ((val & (mask << i)) == val)
> >         break;
> >
> >     if (i == 0)
> >       puts("Fail");
> >
> >     return 0;
> > }
> > ----
> >
> > The problem seem to be that "if (i==0)" is always true!
> 
> try_combine, presented with the following three instructions:
> 
> (gdb) p debug_rtx (i1)
> 
> (insn 12 10 15 (set (reg/v:SI 94)
>         (mem/s:SI (lo_sum:SI (reg/f:SI 95)
>                 (const:SI (plus:SI (symbol_ref:SI ("operands___0"))
>                         (const_int 4 [0x4])))) 2)) 68 {*pa.md:2088}
> (insn_list 10 (nil))
>     (expr_list:REG_DEAD (reg/f:SI 95)
>         (nil)))
> 
> (gdb) p debug_rtx (i2)
> 
> (insn 90 87 91 (set (reg:SI 109)
>         (zero_extend:SI (subreg:QI (reg/v:SI 94) 0))) 131 {*pa.md:3348}
> (insn_list 12 (nil))
>     (nil))
> 
> (gdb) p debug_rtx (i3)
> 
> (jump_insn 91 90 103 (set (pc)
>         (if_then_else (eq (reg:SI 109)
>                 (reg/v:SI 94))
>             (label_ref 47)
>             (pc))) 44 {*pa.md:1497} (insn_list 90 (nil))
>     (expr_list:REG_DEAD (reg:SI 109)
>         (expr_list:REG_BR_PROB (const_int 3999 [0xf9f])
>             (nil))))
> 
> substitutes for the "eq" in the "jump_insn" to get:
> 
> (eq (subreg:SI (mem/s:QI (plus:SI (lo_sum:SI (reg/f:SI 95)
>                     (const:SI (plus:SI (symbol_ref:SI ("operands___0"))
>                             (const_int 4 [0x4]))))
>                 (const_int 3 [0x3])) 2) 0)
>     (mem/s:SI (lo_sum:SI (reg/f:SI 95)
>             (const:SI (plus:SI (symbol_ref:SI ("operands___0"))
>                     (const_int 4 [0x4])))) 2))
> 
> A little further down the track, this code in simplify_comparison thinks
> that the only interesting part of the "eq" operands is the mem:QI, and
> decides that the operands are always equal.
> 
>   /* Now make any compound operations involved in this comparison.
> Then,
>      check for an outmost SUBREG on OP0 that is not doing anything or is
>      paradoxical.  The latter case can only occur when it is known that
> the
>      "extra" bits will be zero.  Therefore, it is safe to remove the
> SUBREG.
>      We can never remove a SUBREG for a non-equality comparison because
> the
>      sign bit is in a different place in the underlying object.  */
> 
>   op0 = make_compound_operation (op0, op1 == const0_rtx ? COMPARE :
> SET);
>   op1 = make_compound_operation (op1, SET);
> 
>   if (GET_CODE (op0) == SUBREG && subreg_lowpart_p (op0)
>       && GET_MODE_CLASS (GET_MODE (op0)) == MODE_INT
>       && (code == NE || code == EQ)
>       && ((GET_MODE_SIZE (GET_MODE (op0))
>            > GET_MODE_SIZE (GET_MODE (SUBREG_REG (op0))))))
>     {
>       op0 = SUBREG_REG (op0);
>       op1 = gen_lowpart_for_combine (GET_MODE (op0), op1);
>     }
> 
> I don't believe that's really where we're going wrong.  The chain of
> replacements is:
>     zero_extend:SI (subreg:QI (reg:SI))
>  -> zero_extend:SI (subreg:QI (mem:SI))
>  -> zero_extend:SI (mem:QI)
>  -> subreg:SI (mem:QI)
> 
> Seems to me that the last replacement is the real problem.
> expand_compound_operation does
> 
>   if (modewidth + len >= pos)
>     tem = simplify_shift_const (NULL_RTX, unsignedp ? LSHIFTRT :
> ASHIFTRT,
>                                 GET_MODE (x),
>                                 simplify_shift_const (NULL_RTX, ASHIFT,
>                                                       GET_MODE (x),
>                                                       XEXP (x, 0),
>                                                       modewidth - pos -
> len),
>                                 modewidth - len);
> 
> and the outer simplify_shift_const gets called with
> 
> Breakpoint 7, simplify_shift_const (x=0x0, code=LSHIFTRT,
> result_mode=SImode,
>     varop=0x40176190, input_count=24) at
> /src/parisc/gcc_new/gcc/combine.c:8970
> (gdb) p debug_rtx (varop)
> 
> (ashift:SI (subreg:SI (mem/s:QI (plus:SI (lo_sum:SI (reg/f:SI 95)
>                     (const:SI (plus:SI (symbol_ref:SI ("operands___0"))
>                             (const_int 4 [0x4]))))
>                 (const_int 3 [0x3])) 2) 0)
>     (const_int 24 [0x18]))
> 
> The two shifts are recognized as being equivalent to an AND with 0xff,
> and we eventually get to
> 
>   if (outer_op != NIL)
>     {
>       if (GET_MODE_BITSIZE (result_mode) < HOST_BITS_PER_WIDE_INT)
>         outer_const = trunc_int_for_mode (outer_const, result_mode);
> 
>       if (outer_op == AND)
>         x = simplify_and_const_int (NULL_RTX, result_mode, x,
> outer_const);
> 
> #0  simplify_and_const_int (x=0x0, mode=SImode, varop=0x40176180,
> constop=255)
>     at /src/parisc/gcc_new/gcc/combine.c:7903
> (gdb) p debug_rtx (varop)
> (subreg:SI (mem/s:QI (plus:SI (lo_sum:SI (reg/f:SI 95)
>                 (const:SI (plus:SI (symbol_ref:SI ("operands___0"))
>                         (const_int 4 [0x4]))))
>             (const_int 3 [0x3])) 2) 0)
> 
> Now, simplify_and_const_int figures that this expression doesn't have
> any
> nonzero bits outside 0xff because of the MEM mode, so there's no need
> for
> an AND at all.  Thus it returns (subreg:SI (mem:QI ...)), which taken in
> isolation is a quite reasonable decision.  So, at a minimum, I think we
> need something like the following:
> 
>         * combine.c (expand_compound_operation): Don't allow
>         simplify_and_const_int to oversimplify zero_extend to a subreg.
> 
> --- combine.c~  Wed Nov  7 11:39:16 2001
> +++ combine.c   Sat Dec 22 23:47:45 2001
> @@ -5781,8 +5781,10 @@ expand_compound_operation (x)
>      return x;
> 
>    /* If we couldn't do this for some reason, return the original
> -     expression.  */
> -  if (GET_CODE (tem) == CLOBBER)
> +     expression.  Also, don't allow eg. (zero_extend:SI (mem:QI ...))
> +     to be replaced by (subreg:SI (mem:QI ...)).  */
> +  if (GET_CODE (tem) == CLOBBER
> +      || (GET_CODE (tem) == SUBREG && XEXP (x, 0) == XEXP (tem, 0)))
>      return x;
> 
>    return tem;
> 
> --
> Alan Modra
> IBM OzLabs - Linux Technology Centre