From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57604 invoked by alias); 11 Oct 2016 09:09:23 -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 57584 invoked by uid 89); 11 Oct 2016 09:09:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.5 required=5.0 tests=BAYES_50,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=facing, U*c, unpatched, stick X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Oct 2016 09:09:12 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id B1039AC47 for ; Tue, 11 Oct 2016 09:09:09 +0000 (UTC) Date: Tue, 11 Oct 2016 09:09:00 -0000 From: Richard Biener To: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix PR77826 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2016-10/txt/msg00687.txt.bz2 On Mon, 10 Oct 2016, Marc Glisse wrote: > On Fri, 7 Oct 2016, Richard Biener wrote: > > > On Thu, 6 Oct 2016, Marc Glisse wrote: > > > > > On Wed, 5 Oct 2016, Richard Biener wrote: > > > > > > > > The following will fix PR77826, the issue that in match.pd matching > > > > > up two things uses operand_equal_p which is too lax about the type > > > > > of the toplevel entity (at least for integer constants). > > > > > > > > > > Bootstrap / regtest pending on x86_64-unknown-linux-gnu. > > > > > > > > The following is what I have applied. > > > > > > > > Richard. > > > > > > > > 2016-10-05 Richard Biener > > > > > > > > PR middle-end/77826 > > > > * genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p > > > > with types_match for GIMPLE code gen to handle type mismatched > > > > constants properly. > > > > > > I don't understand the disparity between generic and gimple here. Why let > > > (char)1 and (long)1 match in generic but not in gimple? And there are > > > probably > > > several transformations in match.pd that could do with an update if > > > constants > > > don't match anymore. Or did I misunderstand what the patch does? > > > > The disparity is mostly that with GENERIC unfolded trees such as (char)1 > > are a bug while in GIMPLE the fact that the match.pd machinery does > > valueization makes those a "feature" we have to deal with. Originally > > I've restricted GENERIC as well but with its types_match_p implementation > > it resulted in too many missed matches. > > I shouldn't have written (long)1, I meant the fact that 1 (as a char constant) > and 1 (as a long constant) will now be matching captures in generic and not in > gimple. If we are going in the direction of not matching constants of > different types, I'd rather do it consistently and update the patterns as > needed to avoid the missed optimizations. The missed matches exist in gimple > as well, and generic optimization seems less important than gimple to me. Yes, I agree. I'll see on looking at the GENERIC fallout in more detail (fixing patterns). > 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) > 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 -- 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. Not sure if this helps enough cases. 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). > 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). > > 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). > > Btw, do you have a better suggestion as to how to handle the original > > issue rather than not relying on operand_equal_p for constants? > > In previous cases, in order to get the right version of a matching capture, we > used non-matching captures and an explicit call to operand_equal_p, for > instance: > > /* X - (X / Y) * Y is the same as X % Y. */ > (simplify > (minus (convert1? @2) (convert2? (mult:c (trunc_div @0 @1) @1))) > /* We cannot use matching captures here, since in the case of > constants we really want the type of @0, not @2. */ > (if (operand_equal_p (@0, @2, 0) > && (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))) > (convert (trunc_mod @0 @1)))) > > That seems to match the solutions that Jakub and you were discussing in the > PR, something like it should work (we can discuss the exact code). Yes. As more of those cases popped up I thought this isn't really a viable way of going forward (to fix the ICEs). So we'll see the above for re-instantiating missed optimizations instead. > I don't know if it is better. The old behavior of matching captures with > inconsistent types was confusing. A behavior with strict matching may > complicate things (will we duplicate patterns, or write operand_equal_p > explicitly to mimic the old behavior?). The recent inconsistency between > generic and gimple doesnt appeal to me much... Yeah, so as first I'm going to fix that disparity. Thanks for the comments, Richard.