On Wed, May 14, 2014 at 4:33 PM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Wed, May 14, 2014 at 12:30 PM, Richard Biener
> <richard.guent...@gmail.com> wrote:
>> On Wed, May 14, 2014 at 12:24 PM, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>> On Tue, May 13, 2014 at 11:06 PM, Prathamesh Kulkarni
>>> <bilbotheelffri...@gmail.com> wrote:
>>>> On Tue, May 13, 2014 at 2:36 PM, Richard Biener
>>>> <richard.guent...@gmail.com> wrote:
>>>>> On Sun, May 11, 2014 at 5:00 PM, Prathamesh Kulkarni
>>>>> <bilbotheelffri...@gmail.com> wrote:
>>>>>> On Sun, May 11, 2014 at 8:10 PM, Andreas Schwab <sch...@linux-m68k.org>
>>>>>> wrote:
>>>>>>> Prathamesh Kulkarni <bilbotheelffri...@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 ...
It appears to be good. However would we want predicates that are
slightly more complex ?
For example, integer_onep @0 ?
I was wondering if it would be a good idea to make predicate an
attribute of operand (expr, capture)
rather than keep predicate as a separate operand ? (since predicates
are only used for checking the operand they are
applied to).
The hierarchy be something like:
struct predicate_operand: public operand
{
predicate_operand (const char *ident_, enum operand::type ty) :
operand (ty), ident (ident_) {}
const char *ident;
virtual void gen_gimple_match (FILE *f, const char *, const char *);
virtual void gen_gimple_transform (FILE *, const char *) = 0;
};
struct capture : public predicate_operand
{
capture (const char *where_, operand *what_, const char *pred = 0)
: predicate_operand (pred, OP_CAPTURE), where (where_), what (what_) {}
const char *where;
operand *what;
virtual void gen_gimple_match (FILE *f, const char *, const char *);
virtual void gen_gimple_transform (FILE *f, const char *);
};
And make capture::gen_gimple_match call
predicate_operand::gen_gimple_match if ident is non-null.
similarly for expr.
>
> Richard.
>
>> Richard.
>>
>>> I'll review the testcase patch separately.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks and Regards,
>>>> Prathamesh
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks and Regards,
>>>>>> Prathamesh
>>>>>>
>>>>>>
>>>>>>> Andreas.
>>>>>>>
>>>>>>> --
>>>>>>> Andreas Schwab, sch...@linux-m68k.org
>>>>>>> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
>>>>>>> "And now for something completely different."
