On Tue, Oct 11, 2016 at 11:34 PM, Marc Glisse <marc.gli...@inria.fr> wrote:
> On Tue, 11 Oct 2016, Bin Cheng wrote:
>
>> We missed folding (convert)(X op const) -> (convert)X op (convert)const
>> for unsigned narrowing because of reason reported at
>> https://gcc.gnu.org/ml/gcc/2016-07/msg00126.html
>> This patch fixes the issue by adding new match&simplify pattern, it also
>> adds a test case. This is the prerequisite patch for next patch adding new
>> vectorization pattern.
>
>
> Some technical comments below. I am sure Jeff and/or Richi will have more to
> say on the approach. I am a bit surprised to see it as adding a new
> transformation, instead of moving an old one.
The "old one" would be c-family/c-common.c:shorten_binary_op. It's generally
prefered to move stuff, preserving semantics.
We do have a similar transform for MULT_EXPR in fold-const.c which also
applies to non-constant 2nd operand (likewise for shorten_binary_op).
The general issue with these "narrowings" is that it loses overflow information
if the original op was carried out in a TYPE_OVERFLOW_UNDEFINED type.
There is also already a bunch of similar match.pd patterns here:
/* Narrowing of arithmetic and logical operations.
These are conceptually similar to the transformations performed for
the C/C++ front-ends by shorten_binary_op and shorten_compare. Long
term we want to move all that code out of the front-ends into here. */
/* If we have a narrowing conversion of an arithmetic operation where
both operands are widening conversions from the same type as the outer
narrowing conversion. Then convert the innermost operands to a suitable
unsigned type (to avoid introducing undefined behavior), perform the
operation and convert the result to the desired type. */
(for op (plus minus)
(simplify
(convert (op:s (convert@2 @0) (convert@3 @1)))
...
so it might be possible to amend these or at least move your pattern next
to those.
> +/* (convert (X op C)) -> ((convert)X op (convert)C) if it is narrowing
> + conversion and both types wrap when overflow. */
> +(for op (plus minus)
> + (simplify
>
> We used to indent by a single space in this file, but I see that other
> transforms have made it in with double spacing, so I guess it doesn't
> matter.
>
> + (convert (op @0 @1))
> + (if (INTEGRAL_TYPE_P (type)
> + && TYPE_OVERFLOW_WRAPS (type)
> + && TREE_CODE (@1) == INTEGER_CST
>
> You can write (convert (op @0 INTEGER_CST@1)) and skip this line.
>
> + && INTEGRAL_TYPE_P (TREE_TYPE (@0))
> + && TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))
>
> This seems quite restrictive, TYPE_OVERFLOW_UNDEFINED should also be fine
> for this type. I guess you are trying to avoid saturating / trapping types?
Maybe he's avoiding the dropping of overflow info ... at least it
warrants a comment.
Richard.
> + && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (@0)))
> + (op (convert @0) (convert @1)))))
> +
> /* If we have a narrowing conversion to an integral type that is fed by a
> BIT_AND_EXPR, we might be able to remove the BIT_AND_EXPR if it merely
> masks off bits outside the final type (and nothing else). */
> diff --git a/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c
> b/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c
> new file mode 100644
> index 0000000..aff96a9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fdump-tree-gimple" } */
> +
> +unsigned char foo (unsigned short s)
> +{
> + return (unsigned char)(s + 65530);
> +}
> +/* { dg-final { scan-tree-dump-not " 65530" "gimple" } } */
>
> As I understand it, C says that s is promoted to int and added to 65530, but
> int is not TYPE_OVERFLOW_WRAPS so your transformation doesn't apply (the
> test already passes without your patch). It is better to write tests for the
> gimple version of transformations, i.e. don't write everything as a single
> expression, use intermediate variables.
>
> --
> Marc Glisse
