Re: [GSoC] writing test-case

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

On Wed, May 14, 2014 at 12:30 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, May 14, 2014 at 12:24 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, May 13, 2014 at 11:06 PM, Prathamesh Kulkarni
>> <bilbotheelffriend@gmail.com> wrote:
>>> On Tue, May 13, 2014 at 2:36 PM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Sun, May 11, 2014 at 5:00 PM, Prathamesh Kulkarni
>>>> <bilbotheelffriend@gmail.com> wrote:
>>>>> On Sun, May 11, 2014 at 8:10 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>>> Prathamesh Kulkarni <bilbotheelffriend@gmail.com> writes:
>>>>>>
>>>>>>> a) I am not able to follow why 3 slashes are required here
>>>>>>> in x_.\\\(D\\\) ? Why does x_.\(D\) not work ?
>>>>>>
>>>>>> Two of the three backslashes are eaten by the tcl parser.  But actually
>>>>>> only two backslashes are needed, since the parens are not special to tcl
>>>>>> (but are special to the regexp engine, so you want a single backslash
>>>>>> surviving the tcl parser).
>>>>>>
>>>>>>> b) The expression after folding would be of the form:
>>>>>>> t2_<digit> = x_<digit>(D) - y_<digit>(D)
>>>>>>> I have used the operator "." in the pattern to match digit.
>>>>>>> While that works in the above case, I think a better
>>>>>>> idea would be to match using [0-9].
>>>>>>> I tried the following but it does not work:
>>>>>>> t_[0-9] = x_[0-9]\\\(D\\\) - y_[0-9]\\\(D\\\)
>>>>>>> Neither does \\\[ and \\\] work.
>>>>>>
>>>>>> Brackets are special in tcl (including inside double quotes), so they
>>>>>> need to be quoted.  But you want the brackets to appear unquoted to the
>>>>>> regexp engine, so a single backslash will do the Right Thing.
>>>>>>
>>>>>> See tcl(n) for the tcl parsing rules.
>>>>>>
>>>>> Thanks. Now I get it, the double backslash \\ is an escape sequence
>>>>> for \, and special characters like (, [
>>>>> retain their meaning in quotes, so to match input text: (D), the
>>>>> pattern has to be written as: "\\(D\\)".
>>>>> I believe "\(D\)" would only match D in the input ?
>>>>> I have modified the test-case. Is this version correct ?
>>>>
>>>> I usually verify that by running the testcase in isolation on a GCC version
>>>> that should FAIL it and on one that should PASS it (tcl quoting is also
>>>> try-and-error for me most of the time...).
>>>>
>>>> Thus I do
>>>>
>>>> gcc/> make check-gcc RUNTESTFLAGS="tree-ssa.exp=match-2.c"
>>>> <test should FAIL>
>>>> <patch source tree>
>>>> gcc/> make cc1
>>>> ... compiles cc1 ...
>>>> gcc/> make check-gcc RUNTESTFLAGS="tree-ssa.exp=match-2.c"
>>>> <test should PASS>
>>>>
>>>> A more complete matching for an SSA name would be (allowing
>>>> for SSA name versions > 9) _\\d\+ with \\(D\\) appended if
>>>> suitable (that's usually known from the testcase).  \\d\+ should match
>>>> at least one decimal digit.
>>> I thought that SSA name version wouldn't exceed 9 for that test-case,
>>> so I decided for matching only one digit. I will change it to match
>>> one or more digits.
>>>
>>> * I have written test-cases for patterns in match.pd (attached patch), which
>>> result in PASS. Could you review them for me ?
>>> I couldn't write for following ones:
>>>
>>> 1] Patterns involving COMPLEX_EXPR, REALPART_EXPR, IMAGPART_EXPR
>>> (match_and_simplify
>>>   (complex (realpart @0) (imagpart @0))
>>>   @0)
>>> (match_and_simplify
>>>   (realpart (complex @0 @1))
>>>   @0)
>>> (match_and_simplify
>>>   (imagpart (complex @0 @1))
>>>   @1)
>>>
>>> Sorry to be daft, but I couldn't understand what these patterns meant
>>> (I know complex numbers).
>>> Could you give an example that matches one of these patterns ?
>>> Thanks.
>>
>> The existing match-1.c testcase has some ideas.  For the first
>> pattern I'd do
>>
>> _Complex double foo (_Complex double z)
>> {
>>   double r = __real z;
>>   double i = __imag z;
>>   return r + 1.0iF * i;
>> }
>>
>> where the return expression is folded (yeah ...) to a COMPLEX_EXPR.
>>
>> For the other two patterns sth like
>>
>> double foo (double r)
>> {
>>   _Complex double z = r;
>>   return __real z;
>> }
>>
>> and
>>
>> double foo (double i)
>> {
>>   _Complex double z = 1.0iF * i;
>>   return __imag z;
>> }
>>
>> should work.
>>
>>> 2] Test-case for FMA_EXPR. I am not sure how to generate FMA_EXPR from C code.
>>> (match_and_simplify
>>>   (fma INTEGER_CST_P@0 INTEGER_CST_P@1 @3)
>>>   (plus (mult @0 @1) @3))
>>
>> I believe it's not possible.  FMA is matched by the optimize_widen_mult
>> pass which runs quite late, after the last forwprop pass.  So I don't think
>> it's possible to write a testcase that triggers with the existing compiler.
>>
>>> 3] Test-case for COND_EXPR
>>> (match_and_simplify
>>>   (cond (bit_not @0) @1 @2)
>>>   (cond @0 @2 @1))
>>>
>>> I believe cond corresponds to C's ternary operator ?
>>> However c-expression of the form:
>>> t2 = (x ? y : z)
>>> gets translated to gimple as an if-else statement, with "x" being condition,
>>> "y" being then-statement, and "z" being else-statement.
>>> So I guess we need to handle this case specially in genmatch ?
>>> Or am I mistaken ?
>>
>> One idea was to also match if-then-else as COND_EXPR (something
>> to explore later), but you can also see COND_EXPRs in the GIMPLE IL,
>> for example they are created by if-conversion which runs before
>> vectorization.  But as above, it's difficult to create a testcase to
>> match on a forwprop transform (those patterns are more likely to
>> match from the various passes code-generation which need to be
>> updated to use the gimple_build interface provided on the breanch).
>>
>> As of the if-then-else idea, for example
>>
>> int foo (int x)
>> {
>>   return x ? 3 : 5;
>> }
>>
>> is seen as
>>
>>   <bb 2>:
>>   if (x_2(D) != 0)
>>     goto <bb 4>;
>>   else
>>     goto <bb 3>;
>>
>>   <bb 3>:
>>
>>   <bb 4>:
>>   # iftmp_1 = PHI <1(2), 0(3)>
>>   return iftmp_1;
>>
>> in GIMPLE SSA form.  Thus the COND_EXPR is translated
>> to a PHI node and a controling predicate.
>>
>> But as said, I'd say we should defer this to a later stage if
>> time permits.
>>
>>> * I added few patterns from TODO list in match.pd. I am
>>> not sure how to write pattern for the following transform:
>>> (T) (P + A) - (T) P -> (T) A
>>
>> This tries to match C pointer difference GIMPLE which is carried
>> out using integer types.  You'd write sth like
>>
>> (match_and_simplify
>>   (minus (convert (pointer_plus @0 @1))
>>              (convert @0))
>>   (convert @1))
>>
>> where a few issues show up.
>>
>> First, in GIMPLE we can
>> have both NOP_EXPR and CONVERT_EXPR which mean the
>> same (and are generally tested with CONVERT_EXRP_P).
>> So the in the patterns we should prefer one of both (I'd say
>> convert) but the code generator has to match both (actually
>> NOP_EXPR appears way more often).
>>
>> Second, for the simplified pattern we generate a convert - but
>> generally we'd want to supply a type to convert to (the current
>> code will simply pick the type of the original expression which
>> is the correct one in this case).  So for other cases I would
>> expect we'd need to write
>>
>> (match_and_simplify
>>   (minus (convert@2 (pointer_plus @0 @1))
>>              (convert @0))
>>   { fold_convert (TREE_TYPE (@2), @1); })
>>
>> thus use C code because there isn't (yet) a way to specify a
>> type for a to generate expression.  I don't expect this to
>> show up very often and thus I didn't look after a syntax
>> to specify types for expression generation or matching
>> as there exists a workaround (by using C expressions for
>> the generation part and an appropriate if-expression for the
>> matching part).
>>
>>> * ICE for transform sqrt (x * x) -> x
>>> The following pattern results in ICE (gimple-match-head.c:546)
>>> (match_and_simplify
>>>   (built_in_sqrt (mult @0 @0))
>>>   @0)
>>>
>>> I triggered the pattern with this test-case:
>>> double foo(double x)
>>> {
>>>   double t1, t2;
>>>   t1 = x * x;
>>>   t2 = sqrt (t1);
>>>   return t2;
>>> }
>>>
>>> This happens because the following assert fails in
>>> bool gimple_match_and_simplify (gimple_stmt_iterator *gsi, tree
>>> (*valueize)(tree)): line 546:
>>> gcc_assert (gimple_seq_singleton_p (tail));
>>>
>>> I guess that happens because for the above pattern
>>> maybe_push_res_to_seq() returns ops[0], and tail remains NULL.
>>>
>>> if (TREE_CODE_LENGTH ((tree_code) rcode) == 0
>>>    && is_gimple_val (ops[0]))
>>>      return ops[0];
>>>
>>> Unfortunately, I couldn't find a fix.
>>
>> Ah, at this point we have a call which we'd like to replace with
>> a SSA name result.  But maybe_push_res_to_seq only pushes
>> if necessary (and for an SSA name or a constant it is not so),
>> but here it's always necessary.
>>
>>> I have a couple of questions regarding gimple-match-head.c
>>>
>>> a) Why do we return if the above condition is true ?
>>> I mean why don't we want gimple_build_assign_with_ops to be called
>>> if that's true ?
>>
>> Because in all other callers this results in more optimal code.
>>
>>> Removing that condition in maybe_push_res_to_seq or calling
>>> gimple_build_assign_with_ops (rcode, lhs, ops[0], NULL_TREE, NULL_TREE,
>>>                                              NULL_TREE);
>>> from gimple_match_and_simplify (if maybe_push_res_to_seq returns ops[0]),
>>> resulted in verify_ssa failed: missing definition.
>>
>> Yeah, I have a fix for that as well.  The call handling code is pretty
>> new and likely needs some fixes.  I'll be available to fix up the
>> details there (they can be tricky).
>>
>> Fix in svn now, btw.
>>
>>> b) In
>>> static bool
>>> gimple_match_and_simplify (gimple stmt,
>>>                            code_helper *rcode, tree *ops,
>>>                            gimple_seq *seq, tree (*valueize)(tree))
>>>
>>> why do we return false by default (line 491), if number of args is
>>> greater than 1 ?
>>
>> Because I didn't implement support for that yet ;)  Support for
>> two arguments is the case 1 cut&pasted and all code handling
>> arg1 duplicated for arg2 as well (and the call to
>> gimple_match_and_simplify gets an additional argument).
>> Not sure if the code generation part is in place yet though.
>>
>> As said above - call handling was written quickly and only late.
>>
>> I'll see to add the two-argument case.
>
> Btw, I have a hard time to find a builtin function where handling more than
> one argument would be useful.  nextafter(3) and friends maybe, but all
> other "mathematical" builtins have a single argument or use further
> arguments for extra return values (which we can't handle).
>
> So I'll leave it as a single argument for now.

pow (x, y) of course.  Added support and also a testcase for it.

So I came along the need to add another predicate for REAL_CST
leafs which makes me wonder if we should support tree codes
as predicates.  Thus instead of writing

(match_and_simplify
  (plus (plus @0 INTEGER_CST_P@1) INTEGER_CST_P@2)
  (plus @0 (plus @1 @2)))

write

(match_and_simplify
  (plus (plus @0 INTEGER_CST@1) INTEGER_CST@2)
  (plus @0 (plus @1 @2)))

we have conveniently choosen lower-case without _EXPR for
expression codes thus the uppercase original form is available
to be used as pre-defined predicate (code generated is
TREE_CODE (x) == <predicate-name>).

Just an idea ...

Richard.

> Richard.
>
>> I'll review the testcase patch separately.
>>
>> Thanks,
>> Richard.
>>
>>> Thanks and Regards,
>>> Prathamesh
>>>>
>>>> Richard.
>>>>
>>>>> Thanks and Regards,
>>>>> Prathamesh
>>>>>
>>>>>
>>>>>> Andreas.
>>>>>>
>>>>>> --
>>>>>> Andreas Schwab, schwab@linux-m68k.org
>>>>>> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
>>>>>> "And now for something completely different."