On Tue, Oct 23, 2018 at 5:01 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 10/23/18 12:20 PM, Richard Biener wrote:
> > On Tue, Oct 23, 2018 at 10:37 AM Martin Liška <mli...@suse.cz> wrote:
> >>
> >> On 10/22/18 4:25 PM, Jakub Jelinek wrote:
> >>> On Mon, Oct 22, 2018 at 04:08:53PM +0200, Martin Liška wrote:
> >>>> Very valid question. I hope as long as I calculate the linear function
> >>>> values in wide_int (get via wi::to_wide (switch_element)), then it should
> >>>> overflow in the same way as original tree type arithmetic. I have a
> >>>> test-case with
> >>>> overflow: gcc/testsuite/gcc.dg/tree-ssa/pr84436-4.c.
> >>>>
> >>>> Do you have any {over,under)flowing test-cases that I should add to
> >>>> test-suite?
> >>>
> >>> I'm worried that the calculation you emit into the code could invoke UB at
> >>> runtime, even if there was no UB in the original code, and later GCC
> >>> passes
> >>> would optimize with the assumption that UB doesn't occur.
> >>> E.g. if the multiplication overflows for one or more of the valid values
> >>> in
> >>> the switch and then the addition adds a negative value so that the end
> >>> result is actually representable.
> >>
> >> In order to address that I verified that neither of (a * x) and (a * x) +
> >> b {over,under}flow
> >> in case of TYPE_OVERFLOW_UNDEFINED (type) is true.
> >>
> >> Hope it's way how to properly make it safe?
> >
> > Hmm, if the default: case is unreachable maybe. But I guess Jakub was
> > suggesting to do the linear function compute in an unsigned type?
> >
> > + /* Let's try to find any linear function a.x + y that can apply to
> >
> > a * x?
>
> Yep.
>
> >
> > + given values. 'a' can be calculated as follows:
> >
> > + tree t = TREE_TYPE (m_index_expr);
> >
> > so unsigned_type_for (TREE_TYPE ...)
> >
> > + tree tmp = make_ssa_name (t);
> > + tree value = fold_build2_loc (loc, MULT_EXPR, t,
> > + wide_int_to_tree (t, coeff_a),
> > + m_index_expr);
> > +
> >
> > + gsi_insert_before (&gsi, gimple_build_assign (tmp, value),
> > GSI_SAME_STMT);
> > + value = fold_build2_loc (loc, PLUS_EXPR, t,
> > + tmp, wide_int_to_tree (t, coeff_b));
> > + tree tmp2 = make_ssa_name (t);
> > + gsi_insert_before (&gsi, gimple_build_assign (tmp2, value),
> > + GSI_SAME_STMT);
> > + load = gimple_build_assign (name, NOP_EXPR, fold_convert (t, tmp2));
> >
> > before the unsigned_type_for that NOP_EXPR would be always redundant.
> >
> > Please also use
> >
> > gimple_seq seq = NULL;
> > tree tmp = gimple_build (&seq, MULT_EXPR, type, ...);
> > tree tmp2 = gimple_build (&seq, PLUS_EXPR, type, ...);
> > tree tmp3 = gimple_convert (&seq, TREE_TYPE (m_index_expr), tmp2);
> > gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
> > load = gimple_build_assign (name, tmp3);
> >
> > not sure why you need the extra assignment at the end, not enough
> > context in the patch.
>
> Thanks for the hint. I did that and tested the patch. It looks fine.
LGTM.
Richard.
> Martin
>
> >
> > Richard.
> >
> >
> >> Martin
> >>
> >>>
> >>> Jakub
> >>>
> >>
>
