2015-09-17 11:12 GMT+02:00 Richard Biener <richard.guent...@gmail.com>: > 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 ...
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