Re: [PATCH] vrp: Fix up gcc.target/aarch64/pr90838.c [PR97312, PR94801]
On 10/8/20 5:08 PM, Jakub Jelinek wrote: On Thu, Oct 08, 2020 at 04:55:07PM +0200, Aldy Hernandez via Gcc-patches wrote: Yes, for max == 0 aka [0, 0] I wanted: 1) if mini == -1, i.e. the DEFINED_VALUE_AT_ZERO == 2 VALUE is -1, return [-1, -1] 2) if maxi == prec, i.e. DEFINED_VALUE_AT_ZERO == 2 VALUE is prec, return [prec, prec] Ah, I see. Do you mind commenting that? Or perhaps you could spell it out obviously like: if (max == 0) { ... if (DEFINED_VALUE_AT_ZERO) // do special things ... } But whatever is fine. I hope to never look at these bits ever again :). Added several comments now (but just in gimple-range.cc, I assume vr-values.c code is what you want to kill eventually). Thanks, and yes... vr-values is on life support :). Aldy
Re: [PATCH] vrp: Fix up gcc.target/aarch64/pr90838.c [PR97312, PR94801]
On Thu, Oct 08, 2020 at 04:55:07PM +0200, Aldy Hernandez via Gcc-patches wrote: > > Yes, for max == 0 aka [0, 0] I wanted: > > 1) if mini == -1, i.e. the DEFINED_VALUE_AT_ZERO == 2 VALUE is -1, return > > [-1, -1] > > 2) if maxi == prec, i.e. DEFINED_VALUE_AT_ZERO == 2 VALUE is prec, return > > [prec, prec] > > Ah, I see. Do you mind commenting that? Or perhaps you could spell it out > obviously like: > > if (max == 0) { > ... > if (DEFINED_VALUE_AT_ZERO) > // do special things > ... > } > > But whatever is fine. I hope to never look at these bits ever again :). Added several comments now (but just in gimple-range.cc, I assume vr-values.c code is what you want to kill eventually). 2020-10-08 Jakub Jelinek PR tree-optimization/94801 PR target/97312 * vr-values.c (vr_values::extract_range_basic) : When stmt is not an internal-fn call or C?Z_DEFINED_VALUE_AT_ZERO is not 2, assume argument is not zero and thus use [0, prec-1] range unless it can be further improved. For CTZ, don't update maxi from upper bound if it was previously prec. * gimple-range.cc (gimple_ranger::range_of_builtin_call) : Likewise. * gcc.dg/tree-ssa/pr94801.c: New test. --- gcc/vr-values.c.jj 2020-10-07 10:47:47.065983121 +0200 +++ gcc/vr-values.c 2020-10-08 15:23:56.042631592 +0200 @@ -1208,34 +1208,42 @@ vr_values::extract_range_basic (value_ra mini = 0; maxi = 1; goto bitop_builtin; - /* __builtin_c[lt]z* return [0, prec-1], except for + /* __builtin_clz* return [0, prec-1], except for when the argument is 0, but that is undefined behavior. -On many targets where the CLZ RTL or optab value is defined -for 0 the value is prec, so include that in the range -by default. */ +Always handle __builtin_clz* which can be only written +by user as UB on 0 and so [0, prec-1] range, and the internal-fn +calls depending on how CLZ_DEFINED_VALUE_AT_ZERO is defined. */ CASE_CFN_CLZ: arg = gimple_call_arg (stmt, 0); prec = TYPE_PRECISION (TREE_TYPE (arg)); mini = 0; - maxi = prec; + maxi = prec - 1; mode = SCALAR_INT_TYPE_MODE (TREE_TYPE (arg)); - if (optab_handler (clz_optab, mode) != CODE_FOR_nothing - && CLZ_DEFINED_VALUE_AT_ZERO (mode, zerov) - /* Handle only the single common value. */ - && zerov != prec) - /* Magic value to give up, unless vr0 proves - arg is non-zero. */ - mini = -2; + if (gimple_call_internal_p (stmt)) + { + if (optab_handler (clz_optab, mode) != CODE_FOR_nothing + && CLZ_DEFINED_VALUE_AT_ZERO (mode, zerov) == 2) + { + /* Handle only the single common value. */ + if (zerov == prec) + maxi = prec; + /* Magic value to give up, unless vr0 proves +arg is non-zero. */ + else + mini = -2; + } + } if (TREE_CODE (arg) == SSA_NAME) { const value_range_equiv *vr0 = get_value_range (arg); /* From clz of VR_RANGE minimum we can compute result maximum. */ if (vr0->kind () == VR_RANGE - && TREE_CODE (vr0->min ()) == INTEGER_CST) + && TREE_CODE (vr0->min ()) == INTEGER_CST + && integer_nonzerop (vr0->min ())) { maxi = prec - 1 - tree_floor_log2 (vr0->min ()); - if (maxi != prec) + if (mini == -2) mini = 0; } else if (vr0->kind () == VR_ANTI_RANGE @@ -1251,9 +1259,14 @@ vr_values::extract_range_basic (value_ra if (vr0->kind () == VR_RANGE && TREE_CODE (vr0->max ()) == INTEGER_CST) { - mini = prec - 1 - tree_floor_log2 (vr0->max ()); - if (mini == prec) - break; + int newmini = prec - 1 - tree_floor_log2 (vr0->max ()); + if (newmini == prec) + { + if (maxi == prec) + mini = prec; + } + else + mini = newmini; } } if (mini == -2) @@ -1261,27 +1274,30 @@ vr_values::extract_range_basic (value_ra goto bitop_builtin; /* __builtin_ctz* return [0, prec-1], except for when the argument is 0, but that is undefined behavior. -If there is a ctz optab for this mode and -CTZ_DEFINED_VALUE_AT_ZERO, include that in the range, -otherwise just assume 0 won't be seen. */ +Always
Re: [PATCH] vrp: Fix up gcc.target/aarch64/pr90838.c [PR97312, PR94801]
On 10/8/20 4:39 PM, Jakub Jelinek wrote: On Thu, Oct 08, 2020 at 04:28:37PM +0200, Aldy Hernandez wrote: On 10/8/20 3:54 PM, Jakub Jelinek wrote: On Thu, Oct 08, 2020 at 12:22:11PM +0200, Jakub Jelinek via Gcc-patches wrote: Perhaps another way out of this would be document and enforce that __builtin_c[lt]z{,l,ll} etc calls are undefined at zero, but C[TL]Z ifn calls are defined there based on *_DEFINED_VALUE_AT_ZERO (*) == 2 Huh, that magic 2 is not obvious. I guess we should document the values of this macro in the source somewhere: The 2 is documented in gccint documentation. BTW. There's no reason why the vr-values can't just call the gimple_ranger::range_of_builtin_call. In the original implementation we had vr-values call the ranger version and trap if they differed. I'm pretty sure you can have vr-values call range_of_builtin_call with a value_range, and things will get squashed down appropriately. We should really only have one version of this. I'm not suggesting you do it, but I wouldn't object to it ;-). Will defer that to you or Andrew ;). I can do it once the dust settles. --- gcc/gimple-range.cc.jj 2020-10-08 11:55:25.498313173 +0200 +++ gcc/gimple-range.cc 2020-10-08 15:36:14.926945183 +0200 @@ -714,8 +730,14 @@ gimple_ranger::range_of_builtin_call (ir // the maximum. wide_int max = r.upper_bound (); if (max == 0) - break; - maxi = wi::floor_log2 (max); + { + if (mini == -1) + maxi = -1; + else if (maxi == prec) + mini = prec; + } + else if (maxi != prec) + maxi = wi::floor_log2 (max); Hmmm, if max == 0, that means the range is [0, 0], because if the highest bound of r is 0, there's nothing left on the bottom but another 0 since R is unsigned. Is that what you meant? Yes, for max == 0 aka [0, 0] I wanted: 1) if mini == -1, i.e. the DEFINED_VALUE_AT_ZERO == 2 VALUE is -1, return [-1, -1] 2) if maxi == prec, i.e. DEFINED_VALUE_AT_ZERO == 2 VALUE is prec, return [prec, prec] Ah, I see. Do you mind commenting that? Or perhaps you could spell it out obviously like: if (max == 0) { ... if (DEFINED_VALUE_AT_ZERO) // do special things ... } But whatever is fine. I hope to never look at these bits ever again :). Aldy
Re: [PATCH] vrp: Fix up gcc.target/aarch64/pr90838.c [PR97312, PR94801]
On Thu, Oct 08, 2020 at 04:28:37PM +0200, Aldy Hernandez wrote: > On 10/8/20 3:54 PM, Jakub Jelinek wrote: > > On Thu, Oct 08, 2020 at 12:22:11PM +0200, Jakub Jelinek via Gcc-patches > > wrote: > > > Perhaps another way out of this would be document and enforce that > > > __builtin_c[lt]z{,l,ll} etc calls are undefined at zero, but C[TL]Z ifn > > > calls are defined there based on *_DEFINED_VALUE_AT_ZERO (*) == 2 > > Huh, that magic 2 is not obvious. I guess we should document the values of > this macro in the source somewhere: The 2 is documented in gccint documentation. > BTW. There's no reason why the vr-values can't just call the > gimple_ranger::range_of_builtin_call. In the original implementation we had > vr-values call the ranger version and trap if they differed. I'm pretty > sure you can have vr-values call range_of_builtin_call with a value_range, > and things will get squashed down appropriately. We should really only have > one version of this. I'm not suggesting you do it, but I wouldn't object to > it ;-). Will defer that to you or Andrew ;). > > --- gcc/gimple-range.cc.jj 2020-10-08 11:55:25.498313173 +0200 > > +++ gcc/gimple-range.cc 2020-10-08 15:36:14.926945183 +0200 > > > @@ -714,8 +730,14 @@ gimple_ranger::range_of_builtin_call (ir > > // the maximum. > > wide_int max = r.upper_bound (); > > if (max == 0) > > - break; > > - maxi = wi::floor_log2 (max); > > + { > > + if (mini == -1) > > + maxi = -1; > > + else if (maxi == prec) > > + mini = prec; > > + } > > + else if (maxi != prec) > > + maxi = wi::floor_log2 (max); > > Hmmm, if max == 0, that means the range is [0, 0], because if the highest > bound of r is 0, there's nothing left on the bottom but another 0 since R is > unsigned. Is that what you meant? Yes, for max == 0 aka [0, 0] I wanted: 1) if mini == -1, i.e. the DEFINED_VALUE_AT_ZERO == 2 VALUE is -1, return [-1, -1] 2) if maxi == prec, i.e. DEFINED_VALUE_AT_ZERO == 2 VALUE is prec, return [prec, prec] 3) otherwise it is an UB case, ignore the argument range, so either [0, prec-1] or VARYING (the latter for the mini == -2 case) The 1) and 2) cases would be well defined, and for 3) I'm worried that e.g. during VRP iteration if at one point we see range of argument say [0, 1] and determine for that [0, prec-1] range, then in another iteration the argument range is narrowed to just [0, 0] and all of sudden the result would become VARYING, I'd be afraid that would be against the rules. Perhaps the right thing is to set range to UNDEFINED in the 3) case. Jakub
Re: [PATCH] vrp: Fix up gcc.target/aarch64/pr90838.c [PR97312, PR94801]
On 10/8/20 3:54 PM, Jakub Jelinek wrote: On Thu, Oct 08, 2020 at 12:22:11PM +0200, Jakub Jelinek via Gcc-patches wrote: Perhaps another way out of this would be document and enforce that __builtin_c[lt]z{,l,ll} etc calls are undefined at zero, but C[TL]Z ifn calls are defined there based on *_DEFINED_VALUE_AT_ZERO (*) == 2 Huh, that magic 2 is not obvious. I guess we should document the values of this macro in the source somewhere: defaults.h: /* Indicate that CLZ and CTZ are undefined at zero. */ #ifndef CLZ_DEFINED_VALUE_AT_ZERO #define CLZ_DEFINED_VALUE_AT_ZERO(MODE, VALUE) 0 #endif #ifndef CTZ_DEFINED_VALUE_AT_ZERO #define CTZ_DEFINED_VALUE_AT_ZERO(MODE, VALUE) 0 #endif The following patch implements that, i.e. __builtin_c?z* now take full advantage of them being UB at zero, while the ifns are well defined at zero if *_DEFINED_VALUE_AT_ZERO (*) == 2. That is what fixes PR94801. Furthermore, to fix PR97312, if it is well defined at zero and the value at zero is prec, we don't lower the maximum unless the argument is known to be non-zero. Heh. I was just fixing the gimple-range.cc version. Thanks. BTW. There's no reason why the vr-values can't just call the gimple_ranger::range_of_builtin_call. In the original implementation we had vr-values call the ranger version and trap if they differed. I'm pretty sure you can have vr-values call range_of_builtin_call with a value_range, and things will get squashed down appropriately. We should really only have one version of this. I'm not suggesting you do it, but I wouldn't object to it ;-). --- gcc/gimple-range.cc.jj 2020-10-08 11:55:25.498313173 +0200 +++ gcc/gimple-range.cc 2020-10-08 15:36:14.926945183 +0200 @@ -714,8 +730,14 @@ gimple_ranger::range_of_builtin_call (ir // the maximum. wide_int max = r.upper_bound (); if (max == 0) - break; - maxi = wi::floor_log2 (max); + { + if (mini == -1) + maxi = -1; + else if (maxi == prec) + mini = prec; + } + else if (maxi != prec) + maxi = wi::floor_log2 (max); Hmmm, if max == 0, that means the range is [0, 0], because if the highest bound of r is 0, there's nothing left on the bottom but another 0 since R is unsigned. Is that what you meant? I think there was a bug in translation here: It looks like the original code did: maxi = tree_floor_log2 (vr0->max ()); /* For vr0 [0, 0] give up. */ if (maxi == -1) break; so perhaps the above (prior to your change) should have been: wide_int max = r.upper_bound (); maxi = wi::floor_log2 (max); // For 0 give up. if (maxi == -1) break; You may want to adjust. Again, thanks for working on this. Aldy
[PATCH] vrp: Fix up gcc.target/aarch64/pr90838.c [PR97312, PR94801]
On Thu, Oct 08, 2020 at 12:22:11PM +0200, Jakub Jelinek via Gcc-patches wrote: > Perhaps another way out of this would be document and enforce that > __builtin_c[lt]z{,l,ll} etc calls are undefined at zero, but C[TL]Z ifn > calls are defined there based on *_DEFINED_VALUE_AT_ZERO (*) == 2 The following patch implements that, i.e. __builtin_c?z* now take full advantage of them being UB at zero, while the ifns are well defined at zero if *_DEFINED_VALUE_AT_ZERO (*) == 2. That is what fixes PR94801. Furthermore, to fix PR97312, if it is well defined at zero and the value at zero is prec, we don't lower the maximum unless the argument is known to be non-zero. For gimple-range.cc I guess we could improve it if needed e.g. by returning a [0,7][32,32] range for .CTZ of e.g. [0,137], but for now it (roughly) matches what vr-values.c does. Ok for trunk if it passes bootstrap/regtest? 2020-10-08 Jakub Jelinek PR tree-optimization/94801 PR target/97312 * vr-values.c (vr_values::extract_range_basic) : When stmt is not an internal-fn call or C?Z_DEFINED_VALUE_AT_ZERO is not 2, assume argument is not zero and thus use [0, prec-1] range unless it can be further improved. For CTZ, don't update maxi from upper bound if it was previously prec. * gimple-range.cc (gimple_ranger::range_of_builtin_call) : Likewise. * gcc.dg/tree-ssa/pr94801.c: New test. --- gcc/vr-values.c.jj 2020-10-07 10:47:47.065983121 +0200 +++ gcc/vr-values.c 2020-10-08 15:23:56.042631592 +0200 @@ -1208,34 +1208,42 @@ vr_values::extract_range_basic (value_ra mini = 0; maxi = 1; goto bitop_builtin; - /* __builtin_c[lt]z* return [0, prec-1], except for + /* __builtin_clz* return [0, prec-1], except for when the argument is 0, but that is undefined behavior. -On many targets where the CLZ RTL or optab value is defined -for 0 the value is prec, so include that in the range -by default. */ +Always handle __builtin_clz* which can be only written +by user as UB on 0 and so [0, prec-1] range, and the internal-fn +calls depending on how CLZ_DEFINED_VALUE_AT_ZERO is defined. */ CASE_CFN_CLZ: arg = gimple_call_arg (stmt, 0); prec = TYPE_PRECISION (TREE_TYPE (arg)); mini = 0; - maxi = prec; + maxi = prec - 1; mode = SCALAR_INT_TYPE_MODE (TREE_TYPE (arg)); - if (optab_handler (clz_optab, mode) != CODE_FOR_nothing - && CLZ_DEFINED_VALUE_AT_ZERO (mode, zerov) - /* Handle only the single common value. */ - && zerov != prec) - /* Magic value to give up, unless vr0 proves - arg is non-zero. */ - mini = -2; + if (gimple_call_internal_p (stmt)) + { + if (optab_handler (clz_optab, mode) != CODE_FOR_nothing + && CLZ_DEFINED_VALUE_AT_ZERO (mode, zerov) == 2) + { + /* Handle only the single common value. */ + if (zerov == prec) + maxi = prec; + /* Magic value to give up, unless vr0 proves +arg is non-zero. */ + else + mini = -2; + } + } if (TREE_CODE (arg) == SSA_NAME) { const value_range_equiv *vr0 = get_value_range (arg); /* From clz of VR_RANGE minimum we can compute result maximum. */ if (vr0->kind () == VR_RANGE - && TREE_CODE (vr0->min ()) == INTEGER_CST) + && TREE_CODE (vr0->min ()) == INTEGER_CST + && integer_nonzerop (vr0->min ())) { maxi = prec - 1 - tree_floor_log2 (vr0->min ()); - if (maxi != prec) + if (mini == -2) mini = 0; } else if (vr0->kind () == VR_ANTI_RANGE @@ -1251,9 +1259,14 @@ vr_values::extract_range_basic (value_ra if (vr0->kind () == VR_RANGE && TREE_CODE (vr0->max ()) == INTEGER_CST) { - mini = prec - 1 - tree_floor_log2 (vr0->max ()); - if (mini == prec) - break; + int newmini = prec - 1 - tree_floor_log2 (vr0->max ()); + if (newmini == prec) + { + if (maxi == prec) + mini = prec; + } + else + mini = newmini; } } if (mini == -2) @@ -1261,27 +1274,30 @@ vr_values::extract_range_basic (value_ra goto bitop_builtin; /* __builtin_ctz* return [0, prec-1], except for when the argument is 0, but that is undefined behavior. -If there is a ctz optab