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