From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6892 invoked by alias); 11 Oct 2016 20:35:42 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 6878 invoked by uid 89); 11 Oct 2016 20:35:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=rop, D*0, simplest, (unknown) X-HELO: mail2-relais-roc.national.inria.fr Received: from mail2-relais-roc.national.inria.fr (HELO mail2-relais-roc.national.inria.fr) (192.134.164.83) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Oct 2016 20:35:31 +0000 Received: from ip-118.net-89-2-234.rev.numericable.fr (HELO laptop-mg.local) ([89.2.234.118]) by mail2-relais-roc.national.inria.fr with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Oct 2016 22:35:28 +0200 Date: Tue, 11 Oct 2016 20:35:00 -0000 From: Marc Glisse Reply-To: gcc-patches@gcc.gnu.org To: Richard Biener cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix PR77826 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset=US-ASCII X-SW-Source: 2016-10/txt/msg00782.txt.bz2 On Tue, 11 Oct 2016, Richard Biener wrote: >> An example that regressed at -O (looking at the .optimized dump) >> >> int f(int a, unsigned b){ >> a &= 1; >> b &= 1; >> return a&b; >> } > > Yeah, but it usually also shows a badly written pattern: > > /* (X & Y) & (X & Z) -> (X & Y) & Z > (X | Y) | (X | Z) -> (X | Y) | Z */ > (for op (bit_and bit_ior) > (simplify > (op:c (convert1?@3 (op:c@4 @0 @1)) (convert2?@5 (op:c@6 @0 @2))) > (if (tree_nop_conversion_p (type, TREE_TYPE (@1)) > && tree_nop_conversion_p (type, TREE_TYPE (@2))) > > so how could we ever match the @0s when we have either of the two > conversions not present? We could only do this then facing constants > (due to using operand_equal_p). We for example fail to handle > > (X & Y) & (unsigned) ((singed)X & Z) Indeed... (oups, looks like I wrote that one) Trying to find other examples /* Fold A - (A & B) into ~B & A. */ (simplify (minus (convert? @0) (convert?:s (bit_and:cs @0 @1))) (if (tree_nop_conversion_p (type, TREE_TYPE (@0)) && tree_nop_conversion_p (type, TREE_TYPE (@1))) (convert (bit_and (bit_not @1) @0)))) Hmm, should be convert1/convert2 to handle the case where @0 is a constant. /* (X | Y) ^ X -> Y & ~ X*/ (simplify (bit_xor:c (convert? (bit_ior:c @0 @1)) (convert? @0)) (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) (convert (bit_and @1 (bit_not @0))))) Again, will never match when there is a convert and @0 is a constant. (for op (bit_and bit_ior bit_xor) rop (bit_ior bit_and bit_and) (simplify (op (convert? (rop:c @0 @1)) (convert? (rop:c @0 @2))) ... Again won't match for constant @0 that has a different type in both parts. /* (X & Y) & Y -> X & Y (X | Y) | Y -> X | Y */ (for op (bit_and bit_ior) (simplify (op:c (convert?@2 (op:c @0 @1)) (convert? @1)) @2)) Same issue. Ok, not many transformations are concerned, and most need a rewrite anyway... In the other direction, looking at the transformations for which we used explicitly operand_equal_p as a workaround for lax integer constant matches, it doesn't look like changing them back to use matching captures would help, it would require duplicating the pattern for constants. >> If we stick to the old behavior, maybe we could have some genmatch magic to >> help with the constant capture weirdness. With matching captures, we could >> select which operand (among those supposed to be equivalent) is actually >> captured more cleverly, either with an explicit marker, or by giving priority >> to the one that is not immediatly below convert? in the pattern. > > This route is a difficult one to take The simplest version I was thinking about was @0 for a true capture, and @@0 for something that just has to be checked for equality with @0. > -- what would be possible is to > capture a specific operand. Like allow one to write > > (op (op @0@4 @1) (op @0@3 @2)) > > and thus actually have three "names" for @0. We have this internally > already when you write > > (convert?@0 @1) > > for the case where there is no conversion. @0 and @1 are the same > in this case. Looks nice and convenient (assuming lax constant matching). > Not sure if this helps enough cases. IIRC, in all cases where we had trouble with operand_equal_p, chosing which operand to capture would have solved the issue. > I still think that having two things matched that are not really > the same is werid (and a possible source of errors as in, ICEs, > rather than missed optimizations). Ok. Let's go with the strict matching, it is indeed less confusing. >> And if we move to stricter matching, maybe genmatch could be updated so >> convert could also match integer constants in some cases. > > You mean when trying to match @0 ... (convert @0) and @0 is an INTEGER_CST > allow then (convert ...) case to match an INTEGER_CST of different type? > That's an interesting idea (not sure how to implement that though). Yes, though I am not sure of the exact semantics that would work best. On a related note, seeing duplicated patterns for constants, I tried several variants of (match (mynot @0) (bit_not @0)) (match (mynot @0) INTEGER_CST@0 (if (@0 = wide_int_to_tree (TREE_TYPE (@0), wi::bit_not (@0))))) (simplify (minus (bit_and:cs @0 (mynot @1)) (bit_and:cs @0 @1)) (minus (bit_xor @0 @1) @1)) This kind of hack feels wrong, but I don't see a proper way to write it. >>> I agree that some transforms would need updates - I've actually tried >>> to implement a warning for genmatch whenever seeing a match with >>> (T)@0 but there isn't any good existing place to sneak that in. >> >> >>>>> * match.pd ((X /[ex] A) * A -> X): Properly handle converted >>>>> and constant A. >>>> >>>> This regressed >>>> int f(int*a,int*b){return 4*(int)(b-a);} >>> >>> This is because (int)(b-a) could be a truncation in which case >>> multiplying with 4 might not result in the same value as >>> b-a truncated(?). The comment before the unpatched patterns >>> said "sign-changing conversions" but nothign actually verified this. >>> Might be that truncations are indeed ok now that I think about it. >> >> 2015-05-22 Marc Glisse >> >> PR tree-optimization/63387 >> * match.pd ((X /[ex] A) * A -> X): Remove unnecessary condition. >> >> Apparently I forgot to remove the comment at that time :-( > > Ok. I'm now testing a patch to remove the restriction again (and adjust > the comment). Thanks. (since exact_div is used almost only with constant divisors, the old pattern was fine before strict matching, but indeed your more general pattern is better) -- Marc Glisse