[patch match.pd c c++]: Ignore results of 'shorten_compare' and move missing patterns in match.pd

Kai Tietz ktietz70@googlemail.com
Thu Sep 17 09:36:00 GMT 2015


2015-09-17 11:12 GMT+02:00 Richard Biener <richard.guenther@gmail.com>:
> On Wed, Sep 16, 2015 at 9:51 PM, Jeff Law <law@redhat.com> wrote:
>> On 09/15/2015 03:42 AM, Kai Tietz wrote:
>>>>>
>>>>> 2015-09-08  Kai Tietz  <ktietz@redhat.com>
>>>>>
>>>>>       * gcc.dg/tree-ssa/vrp23.c: Adjust testcase to reflect that
>>>>>       pattern is matching now already within forward-propagation pass.
>>>>>       * gcc.dg/tree-ssa/vrp24.c: Likewise.
>>>>
>>>>
>>>> So for the new patterns, I would have expected testcases to ensure
>>>> they're
>>>> getting used.
>>>
>>>
>>> We were talking about that.  My approach was to disable shortening
>>> code and see what regressions we get.  For C++ our test-coverage isn't
>>> that good, as we didn't had here any regressions, but for C testcases
>>> we got some.  Eg the testcase vrp25.c is one of them
>>
>> But as I noted, I think that simply looking at testsuite regressions after
>> disabling shorten_compare is not sufficient as I don't think we have
>> significant coverage of this code.
>>
>>>
>>> By disabling "shorten_compare" one of most simple testcases, which is
>>> failing, is:
>>>
>>> int foo (short s, short t)
>>> {
>>>    int a = (int) s;
>>>    int b = (int) t;
>>>
>>>    return a >= b;
>>> }
>>
>> And I would argue it's precisely this kind of stuff we should be building
>> tests around and resolving prior to actually disabling shorten_compare.
>>
>>
>>>
>>> Where we miss to do narrow down SSA-tree comparison to simply s >= t;
>>> If we disable fre-pass ( -fno-tree-fre) for current trunk, we can see
>>> that this pattern gets resolved in forward-propagation pass due
>>> invocation of shorten_compare.
>>>
>>> Another simple one (with disabled "shorten_compare") is:
>>>
>>> The following testcase:
>>>
>>> int foo (unsigned short a)
>>> {
>>>    unsigned int b = 0;
>>>    return (unsigned int) a) < b;
>>> }
>>>
>>> Here we lacked the detection of ((unsigned int) a) < b being a < 0
>>> (which is always false).
>>
>> And again, this deserves a test and resolving prior to disabling
>> shorten_compare.
>>
>>
>>
>>>
>>>> In *theory* one ought to be able to look at the dumps or .s files before
>>>> after this patch for a blob of tests and see that nothing significant has
>>>> changed.  Unfortunately, so much changes that it's hard to evaluate if
>>>> this
>>>> patch is a step forward or a step back.
>>>
>>>
>>> Hmm, actually we deal here with obvious patterns from
>>> "shorten_compare".  Of interest are here the narrowing-lines on top of
>>> this function, which we need to reflect in match.pd too, before we can
>>> deprecate it. I don't get that there are so much changes?  This are in
>>> fact just 3 basic patterns '(convert) X <cmp> (convert) Y',
>>> '((convert) X) <cmp> CST', and a more specialized variant for the last
>>> pattern for '==/!=' case.
>>>
>>>> I wonder if we'd do better to first add new match.pd patterns, one at a
>>>> time, with tests, and evaluating them along the way by looking at the
>>>> dumps
>>>> or .s files across many systems.  Then when we think we've got the right
>>>> set, then look at what happens to those dumps/.s files if we make the
>>>> changes so that shorten_compare really just emits warnings.
>>>
>>>
>>> As the shorten_compare function covers those transformations, I don't
>>> see that this is something leading to something useful.  As long as we
>>> call shorten_compare on folding in FEs, we won't see those patterns in
>>> ME that easy.  And even more important is, that patterns getting
>>> resolved if function gets invoked.
>>
>> It will tell you what will be missed from a code generation standpoint if
>> you disable shorten_compare.  And that is the best proxy we have for
>> performance regressions related to disabling shorten_compare.
>>
>> ie, in a local tree, you disable shorten_compare.  Then compare the code we
>> generate with and without shorten compare.  Reduce to a simple testcase.
>> Resolve the issue with a match.pd pattern (most likely) and repeat the
>> process.  In theory at each step the  number of things to deeply investigate
>> should be diminishing while at the same time you're building match.pd
>> patterns and tests we can include in the testsuite. And each step is likely
>> a new patch in the patch series -- the final patch of which disables
>> shorten_compare.
>>
>> It's a lot of work, but IMHO, it's necessary to help ensure we don't have
>> code generation regressions.
>
> As said in the other reply I successfully used gcc_unreachable ()s in
> code in intend to remove and then inspect/fix all fallout from that during
> bootstrap & test ...

Yes, that would be fine, if shorten_compare would be part of classical
fold-machinery.  But it use isn't there.  It gets just used in
frontend's binary-operation generation in C/C++'s build_binary_op
function.
As we don't want (and can) remove shorten_compare completely, due we
still need atleast its diagnostics,  using of gcc_unreachable ()
doesn't look suitable.
Also the request to remove this function later makes testing of
standard AST-cases not much easy, as we won't see in GIMPLE patterns
already handled by it before.

> Yeah, it's a lot of work (sometimes).  But it's way easier than to investigate
> code changes (which may also miss cases as followup transforms may end
> up causing the same code to be generated).

It would be indeed easier to have such way, but for shorten_compare
(and same applies somewhat to shorten_binary too) this isn't working.

> Richard.

I can add some explicit testcases handled by shorten_compare right
now.  I can model for some cases testcases doing same transformation
in ME (normally done now by vrp/fre), and see that we match them in
forward-propagation already.


Kai

>>
>>
>>
>>>
>>> So I would suggest here to disable shorten_compare invocation and
>>> cleanup with fallout detectable in C-FE's testsuite.
>>
>> But without deeper analysis at the start & patches+testcases for those
>> issues, we have a real risk of regressing the code we generate.
>>
>>>
>>>> My worry is that we get part way through the conversion and end up with
>>>> the
>>>> match.pd patterns without actually getting shorten_compare cleaned up and
>>>> turned into just a warning generator.
>>>
>>>
>>> This worry I share, therefore I see it as mandatory to remove with
>>> initial patch the use of result of shorten_compare, and better cleanup
>>> possible detected additional fallout for other targets.  I just did
>>> regression-testing for Intel-architecture (32-bit and 64-bit).
>>
>> I would disagree that removing shorten_compare should come first.  I think
>> you have to address the code generation regressions.    And I think that the
>> patches to address the code generation regressions need to be a series of
>> patches with testcases.
>>
>> What I don't want is a mega-patch that adds a ton of new patterns to
>> match.pd with minimal/no tests and disables shorten_compare.  We should be
>> able to build up an incremental series of patches that ends with disabling
>> of shorten_compare and with some degree of confidence that we're not badly
>> regressing the code generation.
>>
>> jeff



More information about the Gcc-patches mailing list