On Fri, May 19, 2017 at 01:57:24PM +0300, Alexander Monakov wrote:
> On Fri, 19 May 2017, Richard Biener wrote:
>
> > On Fri, 19 May 2017, Marek Polacek wrote:
> >
> > > On Fri, May 19, 2017 at 09:58:45AM +0200, Richard Biener wrote:
> > > > On Fri, 19 May 2017, Marek Polacek wrote:
> > > >
> > > > > extract_muldiv folds
> > > > >
> > > > > (n * 10000 * z) * 50
> > > > >
> > > > > to
> > > > >
> > > > > (n * 500000) * z
> > > > >
> > > > > which is a wrong transformation to do, because it may introduce an
> > > > > overflow.
> > > > > This resulted in a ubsan false positive. So we should just disable
> > > > > this
> > > > > folding altogether. Does the approach I took make sense?
>
> I think it's possible to keep this folding, note that it's valid to transform
> to
>
> (n * 1 * z) * 500000
>
> (i.e. accumulate multiplications on the outermost factor)
>
> > > > >
> > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> > > >
> > > > Didn't dig very far to identify extract_muldiv, but I guess it's either
> > > > of the following recursions that trigger?
> > > >
> > > > /* If we can extract our operation from the LHS, do so and return
> > > > a
> > > > new operation. Likewise for the RHS from a MULT_EXPR.
> > > > Otherwise,
> > > > do something only if the second operand is a constant. */
> > > > if (same_p
> > > > && (t1 = extract_muldiv (op0, c, code, wide_type,
> > > > strict_overflow_p)) != 0)
> > > > return fold_build2 (tcode, ctype, fold_convert (ctype, t1),
> > > > fold_convert (ctype, op1));
> > > > else if (tcode == MULT_EXPR && code == MULT_EXPR
> > > > && (t1 = extract_muldiv (op1, c, code, wide_type,
> > > > strict_overflow_p)) != 0)
> > > > return fold_build2 (tcode, ctype, fold_convert (ctype, op0),
> > > > fold_convert (ctype, t1));
> > >
> > > Exactly. extract_muldiv first gets (n * 10000 * z) * 50 so it tries
> > > to fold 50 with (subexpressions) of (n * 10000 * z). So it then tries
> > > (n * 10000) * 50, and then n * 50 and then 10000 * 50 which finally
> > > works out, so it uses 50000 and removes the outermost multiplication.
>
> so would it be possible to adjust things here to remove the innermost
> multiplication instead?
I think I'd rather not expand this function any more, sorry.
Marek
