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

[Kai Tietz]

2015-09-15 0:45 GMT+02:00 Jeff Law <law@redhat.com>:
> On 09/08/2015 05:17 AM, Kai Tietz wrote:
>>
>> Hi,
>>
>> This patch is the first part of obsoleting 'shorten_compare' function
>> for folding.
>> It adjusts the uses of 'shorten_compare' to ignore folding returned by
>> it, and adds
>> missing pattterns to match.pd to allow full bootstrap of C/C++ without
>> regressions.
>> Due we are using 'shorten_compare' for some diagnostic we can't simply
>> remove it.  So if this patch gets approved, the next step will be to
>> rename the function to something like 'check_compare', and adjust its
>> arguments and inner logic to reflect that we don't modify
>> arguments/expression anymore within that function.
>>
>> Bootstrap just show 2 regressions within gcc.dg testsuite due patterns
>> matched are folded more early by forward-propagation.  I adjusted
>> them, and added them to patch, too.
>>
>> I did regression-testing for x86_64-unknown-linux-gnu.
>>
>> ChangeLog
>>
>> 2015-09-08  Kai Tietz  <ktietz@redhat.com>
>>
>>      * match.pd: Add missing patterns from shorten_compare.
>>      * c/c-typeck.c (build_binary_op): Discard foldings of
>> shorten_compare.
>>      * cp/typeck.c (cp_build_binary_op): Likewise.
>>
>> 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

> The fact that we're not regressing with the front-end specific shortening
> disabled like this is probably more of an artifact of lack of testing of
> this feature than anything.

This is just true for C++.  For C case we see additional fallout also
in gcc.target specific patterns in i386.  They were mainly about
"blend", and some AVX-testcases.

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;
}

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).

> 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.

So I would suggest here to disable shorten_compare invocation and
cleanup with fallout detectable in C-FE's testsuite.

> 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).

> jeff
>

Kai