On Wed, Sep 16, 2015 at 9:51 PM, Jeff Law <l...@redhat.com> wrote:
> On 09/15/2015 03:42 AM, Kai Tietz wrote:
>>>>
>>>> 2015-09-08 Kai Tietz <kti...@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 ...
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).
Richard.
>
>
>
>>
>> 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
