Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums
On Thu, Apr 9, 2020 at 4:23 AM bin.cheng wrote: > > -- > Sender:Richard Biener > Sent At:2020 Mar. 20 (Fri.) 18:12 > Recipient:bin.cheng > Cc:Andrew Pinski ; GCC Patches > 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 > wrote: > > > > -- > > Sender:Richard Biener > > Sent At:2020 Mar. 3 (Tue.) 17:36 > > Recipient:Andrew Pinski > > Cc:bin.cheng ; GCC Patches > > > > 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 wrote: > > > > > > On Mon, Mar 2, 2020 at 1:40 AM Richard Biener > > > wrote: > > > > > > > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng > > > > 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 > Richard Biener > > 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 > > PR tree-optimization/93674 > * g++.dg/pr93674.C: New test.
Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums
-- Sender:Richard Biener Sent At:2020 Mar. 20 (Fri.) 18:12 Recipient:bin.cheng Cc:Andrew Pinski ; GCC Patches 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 wrote: > > -- > Sender:Richard Biener > Sent At:2020 Mar. 3 (Tue.) 17:36 > Recipient:Andrew Pinski > Cc:bin.cheng ; GCC Patches > > 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 wrote: > > > > On Mon, Mar 2, 2020 at 1:40 AM Richard Biener > > wrote: > > > > > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng > > > 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? Thanks, bin 2020-04-09 Bin Cheng Richard Biener 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 PR tree-optimization/93674 * g++.dg/pr93674.C: New test. 0001-Add-unsigned-type-iv_cand-for-iv_use-with-non-mode-p.patch Description: Binary data
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 wrote: > > -- > Sender:Richard Biener > Sent At:2020 Mar. 3 (Tue.) 17:36 > Recipient:Andrew Pinski > Cc:bin.cheng ; GCC Patches > > 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 wrote: > > > > On Mon, Mar 2, 2020 at 1:40 AM Richard Biener > > wrote: > > > > > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng > > > 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 > > 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 > > PR tree-optimization/93674 > * g++.dg/pr93674.C: New test.
Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums
-- Sender:Richard Biener Sent At:2020 Mar. 3 (Tue.) 17:36 Recipient:Andrew Pinski Cc:bin.cheng ; GCC Patches 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 wrote: > > On Mon, Mar 2, 2020 at 1:40 AM Richard Biener > wrote: > > > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng > > 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. Thanks, bin 2020-03-19 Bin Cheng 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. gcc/testsuite 2020-03-19 Bin Cheng PR tree-optimization/93674 * g++.dg/pr93674.C: New test. 0001-Don-t-add-iv_cand-for-iv_use-with-non-mode-precision.patch Description: Binary data
Fwd: [PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums
Forwarding to public list. -- Forwarded message - From: Bin.Cheng Date: Mon, Mar 9, 2020 at 5:07 PM Subject: Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums To: Richard Biener On Tue, Mar 3, 2020 at 5:36 PM Richard Biener wrote: > > On Mon, Mar 2, 2020 at 6:14 PM Andrew Pinski wrote: > > > > On Mon, Mar 2, 2020 at 1:40 AM Richard Biener > > wrote: > > > > > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng > > > 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. Thank all of you for the suggestions, I will update the change. > > Richard. > > > Thanks, > > Andrew > > > > > > > > + /* Check if cand can represent values of use for strict enums. */ > > > + else if (TREE_CODE (ctype) == ENUMERAL_TYPE && flag_strict_enums) > > > +{ > > > > > > if we don't have enum-typed IV candidates then the computation should > > > be carried out in INTEGER_TYPE and then be converted to enum type. > > > So why's this and the may_eliminate_iv hunks necessary? I was wondering if enum variable is used directly as loop variable? Note above check only happens for adding candidate for derived iv use. Thanks, bin > > > > > > Richard. > > > > > > > Thanks, > > > > bin > > > > 2020-03-02 Bin Cheng > > > > > > > > PR tree-optimization/93674 > > > > * tree-ssa-loop-ivopts.c (add_iv_candidate_for_use): Add > > > > candidate > > > > for enumeral type iv_use converted from other iv. > > > > (get_computation_cost, may_eliminate_iv): Avoid compute, > > > > eliminate > > > > iv_use with enumeral type iv_cand in case of -fstrict-enums. > > > > > > > > gcc/testsuite > > > > 2020-03-02 Bin Cheng > > > > > > > > PR tree-optimization/93674 > > > > * g++.dg/pr93674.C: New test.
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 wrote: > > On Mon, Mar 2, 2020 at 1:40 AM Richard Biener > wrote: > > > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng > > 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. Richard. > Thanks, > Andrew > > > > > + /* Check if cand can represent values of use for strict enums. */ > > + else if (TREE_CODE (ctype) == ENUMERAL_TYPE && flag_strict_enums) > > +{ > > > > if we don't have enum-typed IV candidates then the computation should > > be carried out in INTEGER_TYPE and then be converted to enum type. > > So why's this and the may_eliminate_iv hunks necessary? > > > > Richard. > > > > > Thanks, > > > bin > > > 2020-03-02 Bin Cheng > > > > > > PR tree-optimization/93674 > > > * tree-ssa-loop-ivopts.c (add_iv_candidate_for_use): Add candidate > > > for enumeral type iv_use converted from other iv. > > > (get_computation_cost, may_eliminate_iv): Avoid compute, eliminate > > > iv_use with enumeral type iv_cand in case of -fstrict-enums. > > > > > > gcc/testsuite > > > 2020-03-02 Bin Cheng > > > > > > PR tree-optimization/93674 > > > * g++.dg/pr93674.C: New test.
Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums
On Mon, Mar 2, 2020 at 1:40 AM Richard Biener wrote: > > On Mon, Mar 2, 2020 at 9:07 AM bin.cheng 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. Thanks, Andrew > > + /* Check if cand can represent values of use for strict enums. */ > + else if (TREE_CODE (ctype) == ENUMERAL_TYPE && flag_strict_enums) > +{ > > if we don't have enum-typed IV candidates then the computation should > be carried out in INTEGER_TYPE and then be converted to enum type. > So why's this and the may_eliminate_iv hunks necessary? > > Richard. > > > Thanks, > > bin > > 2020-03-02 Bin Cheng > > > > PR tree-optimization/93674 > > * tree-ssa-loop-ivopts.c (add_iv_candidate_for_use): Add candidate > > for enumeral type iv_use converted from other iv. > > (get_computation_cost, may_eliminate_iv): Avoid compute, eliminate > > iv_use with enumeral type iv_cand in case of -fstrict-enums. > > > > gcc/testsuite > > 2020-03-02 Bin Cheng > > > > PR tree-optimization/93674 > > * g++.dg/pr93674.C: New test.
Re: [PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums
On Mon, Mar 2, 2020 at 9:07 AM bin.cheng 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. + /* Check if cand can represent values of use for strict enums. */ + else if (TREE_CODE (ctype) == ENUMERAL_TYPE && flag_strict_enums) +{ if we don't have enum-typed IV candidates then the computation should be carried out in INTEGER_TYPE and then be converted to enum type. So why's this and the may_eliminate_iv hunks necessary? Richard. > Thanks, > bin > 2020-03-02 Bin Cheng > > PR tree-optimization/93674 > * tree-ssa-loop-ivopts.c (add_iv_candidate_for_use): Add candidate > for enumeral type iv_use converted from other iv. > (get_computation_cost, may_eliminate_iv): Avoid compute, eliminate > iv_use with enumeral type iv_cand in case of -fstrict-enums. > > gcc/testsuite > 2020-03-02 Bin Cheng > > PR tree-optimization/93674 > * g++.dg/pr93674.C: New test.
[PATCH PR93674]Avoid introducing IV of enumeral type in case of -fstrict-enums
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? Thanks, bin 2020-03-02 Bin Cheng PR tree-optimization/93674 * tree-ssa-loop-ivopts.c (add_iv_candidate_for_use): Add candidate for enumeral type iv_use converted from other iv. (get_computation_cost, may_eliminate_iv): Avoid compute, eliminate iv_use with enumeral type iv_cand in case of -fstrict-enums. gcc/testsuite 2020-03-02 Bin Cheng PR tree-optimization/93674 * g++.dg/pr93674.C: New test. pr93674-20200302.txt Description: Binary data