Re: Division Optimization in match and simplify

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Nov 4, 2015 at 12:18 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Wed, 4 Nov 2015, Hurugalawadi, Naveen wrote:
>
>>>> I thought we were mostly using the 'convert?'
>>>> and tree_nop_conversion_p on integers

Yes, on floats they shouldn't be used.

>>
>> Done. Cleared all instances of convert which are not required.
>> However, I am still confused about the use of "convert" in match
>> and simplify.
>
>
> It could be that I am wrong, you'll have to wait for Richard's comments
> anyway. The one place where a convert could be useful is:
> (div (convert? (bit_and @0 INTEGER_CST@1)) INTEGER_CST@2)
> but I didn't check if we want it (it would probably at least require
> (convert @0) instead of plain @0 in the result so the division and the shift
> are done with the same signedness).
>
>>>> So all patterns looking at arg[01] are handling nop-conversions on their
>>>> outermost operands while those looking at op[01] do not.
>>
>>
>> These patterns are looking at arg[01] rather than op[01] ?
>
>
> Might be a case where it doesn't really matter which one you look at, and
> the easiest one in fold-const may not be the same as in match.pd.
>
> +/* Convert (A/B)/C to A/(B*C)  */
> +(simplify
> + (rdiv (rdiv @0 @1) @2)
> +  (if (flag_reciprocal_math)
>
> I would indent this line the same as the one above.

Yep.

> +   (rdiv @0 (mult @1 @2))))
> +
> +/* Convert A/(B/C) to (A/B)*C  */
> +(simplify
> + (rdiv @0 (rdiv @1 @2))
> +  (if (flag_reciprocal_math)
> +   (mult (rdiv @0 @1) @2)))
>
> I believe you might want a :s on the inner rdiv (?).
> Note that you can factor out the test on flag_reciprocal_math so you write
> it only once for 2 patterns.

Please use :s on both inner rdiv in both patterns.  With that the two patterns
are ok.

+/* Convert C1/(X*C2) into (C1/C2)/X  */
+(simplify
+ (rdiv (REAL_CST@0) (mult @1 REAL_CST@2))

Omit the parens around REAL_CST@0

+  (if (flag_reciprocal_math)
+   (with
+    { tree tem = const_binop (RDIV_EXPR, type, @0, @2); }
+    (if (tem)
+     (rdiv { tem; } @1)))))

with that the pattern is ok.

>
> I don't really remember what the tests !TYPE_UNSIGNED (type) and
> tree_int_cst_sgn are for in the other pattern, but since you are only moving
> the transformation...

+/* Optimize (X & (-A)) / A where A is a power of 2, to X >> log2(A) */
+(for div (exact_div trunc_div)
+ (simplify
+  (div (bit_and @0 INTEGER_CST@1) INTEGER_CST@2)
+  (if (!TYPE_UNSIGNED (type) && integer_pow2p (@2)
+       && tree_int_cst_sgn (@2) > 0
+       && wi::add (@2, @1) == 0)
+   (rshift @0 { build_int_cst (integer_type_node, wi::exact_log2 (@2)); }))))

the TYPE_UNSIGNED test is because right shift of negative values is undefined,
so is a shift with a negative value.  I believe we can safely handle
conversions here
just like fold-const.c does with

 (div (convert? (bit_and @0 INTEGER_CST@1) INTEGER_CST@2)
 (if (tree_nop_conversion_p (type, TREE_TYPE (@0)))
  ...

With that the pattern looks ok to me.

Richard.


>
> --
> Marc Glisse