On Tue, Nov 11, 2014 at 7:52 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Tue, Nov 11, 2014 at 4:52 AM, Patrick Palka <patr...@parcs.ath.cx> wrote: >> This patch refactors the VRP edge-assertion code to make it always >> traverse SSA-name definitions in order to find suitable edge assertions >> to insert. Currently SSA-name definitions get traversed only when the >> LHS of the original conditional is a bitwise AND or OR operation which >> seems like a strange restriction. We should always try to traverse >> the SSA-name definitions inside the conditional, in particular for >> conditionals with the form: >> >> int p = x COMP y; >> if (p != 0) -- edge assertion: x COMP y > > Of course this specific case should have been simplified to > > if (x COMP y) > > if that comparison cannot trap and -fnon-call-exceptions is in effect.
Like Andrew said, I noticed that if p is shared then such comparisons don't get simplified. And like in the case of uninit-pred-9_b.c it seems that the compiler sometimes implicitly CSEs duplicate conditionals. > >> To achieve this the patch merges the mutually recursive functions >> register_edge_assert_for_1() and register_edge_assert_for_2() into a >> single recursive function, register_edge_assert_for_1(). In doing so, >> code duplication can be reduced and at the same time the more general >> logic allows VRP to detect more useful edge assertions. >> >> The recursion of the function register_edge_assert_for_1() is bounded by >> a new 'limit' argument which is arbitrarily set to 4 so that at most 4 >> levels of SSA-name definitions will be traversed per conditional. >> (Incidentally this hard recursion limit makes the related fix for PR >> 57685 unnecessary.) >> >> A test in uninit-pred-9_b.c now has to be marked xfail because in it VRP >> (correctly) transforms the statement >> >> # prephitmp_35 = PHI <pretmp_9(8), _28(10)> >> into >> # prephitmp_35 = PHI <pretmp_9(8), 1(10)> >> >> and the uninit pass doesn't properly handle such PHIs containing a >> constant value as one of its arguments -- so a bogus uninit warning is >> now emitted. > > Did you try fixing that? It seems to me a constant should be easy > to handle? I tried a couple months ago and I failed. I might try again. > >> Full bootstrap + regtesting on x86_64-unknown-linux-gnu is in progress. >> Is it OK to commit if testing finishes with no new regressions? > > Ok. I decided to replace this refactoring patch with a simpler patch (sent to the ML) that just changes (adds) about 20LOC to tree-vrp.c. The patch is not as extensive (a few of the tests in vrp-2.c still fail) but I am more comfortable about the patch's correctness and its impact on compile time. Sorry, I should've been more clear about that.
