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.
Was there any reason to handle this special case? Why not simply
do sth like
+ 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)
Thanks,
Richard.
> Thanks,
> bin
> 2020-03-19 Bin Cheng <bin.ch...@linux.alibaba.com>
>
> PR tree-optimization/93674
> * tree-ssa-loop-ivopts.c (add_iv_candidate_for_use): Don't add iv_cand
> for iv_use which is not integer or pointer type, or non-mode precision
> type. For such cases, add iv_cand for source operand if it's
> converted from other one.
> (get_computation_cost, may_eliminate_iv): Avoid compute, eliminate
> iv_use with enumeral type iv_cand in case of -fstrict-enums.
That change is no longer in the patch.
>
> gcc/testsuite
> 2020-03-19 Bin Cheng <bin.ch...@linux.alibaba.com>
>
> PR tree-optimization/93674
> * g++.dg/pr93674.C: New test.
