| Message ID | 568FDB72.70109@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 8 Jan 2016, Matthew Wahab wrote: > Hello, > > The C/C++ front-ends apply type conversions to expressions using ABS > with integral arguments of type smaller than int. This means that, for > short x, ABS(x) becomes something like (short)ABS((int)x)). When the > argument is the result of memory access, this restricts the vectorizer > apparently because the alignment of the operation doesn't match the > alignment of the memory access. This causes two failures in > gcc.target/aarch64/vec-abs-compile.c where the expected abs instructions > aren't generated. > > This patch adds a case to convert_to_integer_1 to push the integer type > conversions applied to an ABS expression into the argument, when it is > safe to do so. This fixes the failing test cases. Note that, for example, (int) labs (INT_MIN) is well-defined if long is wider than int (in GNU C, where conversion of out-of-range values to signed types is well-defined). But abs (INT_MIN) is undefined. That is, such transformations can never be safe unless the minimum value of the type of the ABS_EXPR argument is not a possible value of the contained expression. What's your definition of safe? I'd expect a patch like this to contain lots of testcases verifying that the transformation occurs in cases where it's OK, but that (int) labs (int_var) does not end up with an ABS_EXPR on type int (for example) (in the absence of a guarantee that said ABS_EXPR will return INT_MIN for an argument of INT_MIN - which might be the case for -fwrapv, but can't be otherwise because *_nonnegative*_p assume ABS_EXPR always produces a nonnegative result).
On Fri, Jan 8, 2016 at 11:24 PM, Joseph Myers <joseph@codesourcery.com> wrote: > On Fri, 8 Jan 2016, Matthew Wahab wrote: > >> Hello, >> >> The C/C++ front-ends apply type conversions to expressions using ABS >> with integral arguments of type smaller than int. This means that, for >> short x, ABS(x) becomes something like (short)ABS((int)x)). When the >> argument is the result of memory access, this restricts the vectorizer >> apparently because the alignment of the operation doesn't match the >> alignment of the memory access. This causes two failures in >> gcc.target/aarch64/vec-abs-compile.c where the expected abs instructions >> aren't generated. >> >> This patch adds a case to convert_to_integer_1 to push the integer type >> conversions applied to an ABS expression into the argument, when it is >> safe to do so. This fixes the failing test cases. > > Note that, for example, (int) labs (INT_MIN) is well-defined if long is > wider than int (in GNU C, where conversion of out-of-range values to > signed types is well-defined). But abs (INT_MIN) is undefined. That is, > such transformations can never be safe unless the minimum value of the > type of the ABS_EXPR argument is not a possible value of the contained > expression. > > What's your definition of safe? I'd expect a patch like this to contain > lots of testcases verifying that the transformation occurs in cases where > it's OK, but that (int) labs (int_var) does not end up with an ABS_EXPR on > type int (for example) (in the absence of a guarantee that said ABS_EXPR > will return INT_MIN for an argument of INT_MIN - which might be the case > for -fwrapv, but can't be otherwise because *_nonnegative*_p assume > ABS_EXPR always produces a nonnegative result). Also please don't add to convert_* and friends but instead amend match.pd with appropriate rules. Richard. > -- > Joseph S. Myers > joseph@codesourcery.com
On 08/01/16 22:24, Joseph Myers wrote: > On Fri, 8 Jan 2016, Matthew Wahab wrote: > >> Hello, >> >> The C/C++ front-ends apply type conversions to expressions using ABS >> with integral arguments of type smaller than int. This means that, for >> short x, ABS(x) becomes something like (short)ABS((int)x)). When the >> argument is the result of memory access, this restricts the vectorizer >> apparently because the alignment of the operation doesn't match the >> alignment of the memory access. This causes two failures in >> gcc.target/aarch64/vec-abs-compile.c where the expected abs instructions >> aren't generated. >> >> This patch adds a case to convert_to_integer_1 to push the integer type >> conversions applied to an ABS expression into the argument, when it is >> safe to do so. This fixes the failing test cases. > > Note that, for example, (int) labs (INT_MIN) is well-defined if long is > wider than int (in GNU C, where conversion of out-of-range values to > signed types is well-defined). But abs (INT_MIN) is undefined. That is, > such transformations can never be safe unless the minimum value of the > type of the ABS_EXPR argument is not a possible value of the contained > expression. The case I'm trying to fix has (short)abs((int)short_var). I'd thought that if abs(short_var) was undefined because the result couldn't be represented then the type conversion from int to short would also be undefined. In fact, it's implementation defined and S4.5 of the GCC manual says that the value is reduced until it can be represented. So (short)abs((int)short_var) will produce a value when abs(short_var) is undefined meaning this transformation isn't correct. I'll drop this patch. Thanks, Matthew > What's your definition of safe? I'd expect a patch like this to contain > lots of testcases verifying that the transformation occurs in cases where > it's OK, but that (int) labs (int_var) does not end up with an ABS_EXPR on > type int (for example) (in the absence of a guarantee that said ABS_EXPR > will return INT_MIN for an argument of INT_MIN - which might be the case > for -fwrapv, but can't be otherwise because *_nonnegative*_p assume > ABS_EXPR always produces a nonnegative result). >
On 01/11/2016 05:33 PM, Matthew Wahab wrote: > > The case I'm trying to fix has (short)abs((int)short_var). I'd thought > that if > abs(short_var) was undefined because the result couldn't be represented > then the type > conversion from int to short would also be undefined. In fact, it's > implementation > defined and S4.5 of the GCC manual says that the value is reduced until > it can be > represented. So (short)abs((int)short_var) will produce a value when > abs(short_var) is undefined meaning this transformation isn't correct. > I'll drop this patch. Maybe we could have an optab and corresponding internal function for an abs that's always defined. Bernd
On January 11, 2016 5:36:33 PM GMT+01:00, Bernd Schmidt <bschmidt@redhat.com> wrote: >On 01/11/2016 05:33 PM, Matthew Wahab wrote: >> >> The case I'm trying to fix has (short)abs((int)short_var). I'd >thought >> that if >> abs(short_var) was undefined because the result couldn't be >represented >> then the type >> conversion from int to short would also be undefined. In fact, it's >> implementation >> defined and S4.5 of the GCC manual says that the value is reduced >until >> it can be >> represented. So (short)abs((int)short_var) will produce a value when >> abs(short_var) is undefined meaning this transformation isn't >correct. >> I'll drop this patch. > >Maybe we could have an optab and corresponding internal function for an > >abs that's always defined. I'd like to have ABSU_EXPR (or allow unsigned result on ABS_EXPR). Richard. > >Bernd
On 11/01/16 17:46, Richard Biener wrote: > On January 11, 2016 5:36:33 PM GMT+01:00, Bernd Schmidt <bschmidt@redhat.com> wrote: >> On 01/11/2016 05:33 PM, Matthew Wahab wrote: >>> >>> The case I'm trying to fix has (short)abs((int)short_var). I'd >> thought >>> that if >>> abs(short_var) was undefined because the result couldn't be >> represented >>> then the type >>> conversion from int to short would also be undefined. In fact, it's >>> implementation >>> defined and S4.5 of the GCC manual says that the value is reduced >> until >>> it can be >>> represented. So (short)abs((int)short_var) will produce a value when >>> abs(short_var) is undefined meaning this transformation isn't >> correct. >>> I'll drop this patch. >> >> Maybe we could have an optab and corresponding internal function for an >> >> abs that's always defined. > > I'd like to have ABSU_EXPR (or allow unsigned result on ABS_EXPR). > I'll see if I can do anything along those lines. This looks like something for stage 1 though. Matthew
diff --git a/gcc/convert.c b/gcc/convert.c index 4b1e1f1..95ff1e2 100644 --- a/gcc/convert.c +++ b/gcc/convert.c @@ -852,6 +852,24 @@ convert_to_integer_1 (tree type, tree expr, bool dofold) } break; + case ABS_EXPR: + /* Convert (N) abs ((W)x) -> abs ((N)x) + if N, W and the type T of x are all signed integer types + and the precision of N is >= the precision of T. */ + { + tree arg0 = get_unwidened (TREE_OPERAND (expr, 0), type); + tree arg0_type = TREE_TYPE (arg0); + + if (!TYPE_UNSIGNED (type) + && !TYPE_UNSIGNED (arg0_type) + && outprec >= TYPE_PRECISION (arg0_type)) + { + return fold_build1 (ABS_EXPR, type, + convert (type, arg0)); + } + break; + } + case NEGATE_EXPR: case BIT_NOT_EXPR: /* This is not correct for ABS_EXPR,