On Thu, Apr 9, 2020 at 4:23 AM bin.cheng <bin.ch...@linux.alibaba.com> wrote:
>
> ------------------------------------------------------------------
> Sender:Richard Biener <richard.guent...@gmail.com>
> Sent At:2020 Mar. 20 (Fri.) 18:12
> Recipient:bin.cheng <bin.ch...@linux.alibaba.com>
> Cc:Andrew Pinski <pins...@gmail.com>; GCC Patches <gcc-patches@gcc.gnu.org>
> Subject:Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of
> -fstrict-enums
>
>
> On Fri, Mar 20, 2020 at 10:27 AM bin.cheng <bin.ch...@linux.alibaba.com>
> wrote:
> >
> > ------------------------------------------------------------------
> > Sender:Richard Biener <richard.guent...@gmail.com>
> > Sent At:2020 Mar. 3 (Tue.) 17:36
> > Recipient:Andrew Pinski <pins...@gmail.com>
> > Cc:bin.cheng <bin.ch...@linux.alibaba.com>; GCC Patches
> > <gcc-patches@gcc.gnu.org>
> > Subject:Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of
> > -fstrict-enums
> >
> >
> > On Mon, Mar 2, 2020 at 6:14 PM Andrew Pinski <pins...@gmail.com> wrote:
> > >
> > > On Mon, Mar 2, 2020 at 1:40 AM Richard Biener
> > > <richard.guent...@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng <bin.ch...@linux.alibaba.com>
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > > This is a simple fix for PR93674. It adds cand carefully for
> > > > > enumeral type iv_use in
> > > > > case of -fstrict-enums, it also avoids computing, replacing iv_use
> > > > > with the candidate
> > > > > so that no IV of enumeral type is introduced with -fstrict-enums
> > > > > option.
> > > > >
> > > > > Testcase is also added. Bootstrap and test on x86_64. Any comment?
> > > >
> > > > I think we should avoid enum-typed (or bool-typed) IVs in general, not
> > > > just
> > > > with -fstrict-enums. That said, the ENUMERAL_TYPE checks should be
> > > > !(INTEGER_TYPE || POINTER_TYPE_P) checks.
> > >
> > > Maybe even check type_has_mode_precision_p or
> > > TYPE_MIN_VALUE/TYPE_MAX_VALUE have the same as the min/max for that
> > > precision/signedness.
> >
> > Indeed we don't want non-mode precision INTEGER_TYPE IVs either. I wouldn't
> > check TYPE_MIN/MAX_VALUE here though.
> >
> > Here is the updated patch with more checks. I also removed the
> > computation/elimination part for now.
>
> + if ((TREE_CODE (basetype) != INTEGER_TYPE && !POINTER_TYPE_P (basetype))
> + || !type_has_mode_precision_p (basetype))
> + {
> + if (!CONVERT_EXPR_P (iv->base))
> + return;
> +
> + /* Specially handle such iv_use if it's converted from other ones by
> + adding candidate for the source iv. */
> + base = TREE_OPERAND (iv->base, 0);
> + basetype = TREE_TYPE (base);
> + if (!INTEGRAL_TYPE_P (basetype))
> + return;
>
> I think this is too lax - you'll happily add another non-INTEGER_TYPE
> or non-mode-precision IV through this handling. If the conversion
> is not value-preserving the IV may also be useless (also the step
> might see truncation in its type adjustment). With a widening
> conversion there's the issue with different wrap-around behavior.
> I guess in most cases we'll not be able to use the IV if it is "bogus"
> so all this might be harmless.
>
> The original idea is to depend on the condition that the result won't be a
> legal IV in
> case of widening. For narrowing, it won't be a problem except what you
> mentioned
> about non-INTEGER_TYPE or non-mode-precision issue.
>
> Was there any reason to handle this special case? Why not simply
> do sth like
> The reason is to strength reduce (conversion) operation in RHS by introducing
> appropriate
> candidate.
> And right, your proposed code looks better and more general.
>
> + if ((TREE_CODE (basetype) != INTEGER_TYPE && !POINTER_TYPE_P (basetype)))
> + || !type_has_mode_precision_p (basetype))
> + {
> basetype = lang_hooks.type_for_mode (TYPE_MODE (basetype),
> TYPE_UNSIGNED (basetype));
> tree step = fold_convert (basetype, iv->step);
> add_candidate (...);
>
> ?
>
> The following also looks bogus:
>
> + if (basetype == generic_type_for (basetype))
> + step = fold_convert (basetype, step);
>
> don't we add IVs in generic_type_for () and thus the conversion is
> always necessary
> anyways? (it has the wrong type from the conversion we just stripped)
>
> This is now discarded.
> Patch updated and tested on x86_64. Is it OK?
OK.
Richard.
> Thanks,
> bin
>
> 2020-04-09 Bin Cheng <bin.ch...@linux.alibaba.com>
> Richard Biener <rguent...@suse.de>
>
> PR tree-optimization/93674
> * tree-ssa-loop-ivopts.c (langhooks.h): New include.
> (add_iv_candidate_for_use): For iv_use of non integer or pointer type,
> or non-mode precision type, add candidate in unsigned type with the
> same precision.
>
> gcc/testsuite
> 2020-04-09 Bin Cheng <bin.ch...@linux.alibaba.com>
>
> PR tree-optimization/93674
> * g++.dg/pr93674.C: New test.
