2015-09-15 0:45 GMT+02:00 Jeff Law <l...@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 <kti...@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 <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 > 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